On Thu, Jan 05, 2017 at 04:51:53PM -0600, Eric Blake wrote:
> On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> > + * <example>
> > + * <title>Resolving addresses synchronously</title>
> > + * <programlisting>
> > + * int mylisten(SocketAddress *addr, Error **errp) {
> > + * QIODNSResolver *resolver = qio_dns_resolver_get_instance();
> > + * SocketAddress **rawaddrs = NULL;
> > + * size_t nrawaddrs = 0;
> > + * Error *err = NULL;
> > + * QIOChannel **socks = NULL;
> > + * size_t nsocks = 0;
> > + *
> > + * if (qio_dns_resolver_lookup_sync(dns, addr, &nrawaddrs,
> > + * &rawaddrs, &err) < 0) {
> > + * error_propagate(errp, err);
>
> You aren't using the local err here; why not just call
> qio_dns_resolver_lookup_sync(, errp) directly, then you don't need to
> propagate?
Yep
> Is it guaranteed that 'err' is set only when
> qio_dns_resolver_lookup_sync() returns negative, and conversely that
> nrawaddrs is > 0 when it succeeded? It matters later...[3]
getaddrinfo() returns an error code of EAI_NODATA if the dns name
has no corresponding records, so we should be guaranteed > 0 for
nrawaddrs on success.
>
> > + * return -1;
> > + * }
> > + *
> > + * for (i = 0; i < nrawaddrs; i++) {
> > + * QIOChannel *sock = qio_channel_new();
> > + * Error *local_err = NULL;
>
> It looks weird that you are declaring two different local Error*
> variables. But I read further down, and finally figured out why...[1]
Yeah, the peculiarty of getaddrinfo where you should only report an
error if every address failed.
>
> > + * qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
> > + * if (local_err && !err) {
> > + * error_propagate(err, local_err);
>
> Won't compile as written; you need error_propagate(&err, local_err);
>
> error_propagate() is safe to call more than once (first error wins), so
> you could simplify this to 'if (local_err) {'. In fact, if you don't
> rewrite the condition, then on the second error, you end up falling
> through to the else...[2]
>
> > + * } else {
> > + * socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
>
> [2]...and allocating a slot in spite of the failure, plus leaking
> local_err at the end of the loop. Oops.
Oh, nice spot.
> As long as nsocks is not going to be arbitrarily huge, it shouldn't
> matter that this particular style of array growth is O(n^2) in
> complexity (quadratic complexity is fine on small lists, but for large
> lists, you need to realloc in geometrically-larger increments to keep
> things amortized to linear costs that otherwise explode due to copying
> old-to-new array on each iteration).
Yeah, realistically 99% of the time there will be 2 results (ipv4 and
ipv6). Only in niche cases where there's a host with multiple public
IPs will we have more addressses and even then it'll be less than
10. So the complexity is not an issue here IMHO - the actual dns
network lookup time will dwarf any inefficiency.
> > + * } else {
> > + * error_free(err);
>
> [1]...and this is why you have two error variables. You've chosen to
> explicitly succeed if you get at least one socket open, even in the case
> where resolution returns multiple possible addresses and some of them fail.
Yep, glibc recommended behaviour for listening with getaddrinfo().
> > +/**
> > + * qio_dns_resolver_lookup_sync:
> > + * @resolver: the DNS resolver instance
> > + * @addr: the address to resolve
> > + * @naddr: pointer to hold number of resolved addresses
> > + * @addrs: pointer to hold resolved addresses
> > + * @errp: pointer to NULL initialized error object
> > + *
> > + * This will attempt to resolve the address provided
> > + * in @addr. If resolution succeeds, @addrs will be filled
> > + * with all the resolved addresses. @naddrs will specify
> > + * the number of entries allocated in @addrs. The caller
> > + * is responsible for freeing each entry in @addrs, as
> > + * well as @addrs itself.
>
> Where in your example code above do you free the memory? Or is that a
> leak you need to plug?
Opps, yes, a leak
> Are we guaranteed that naddrs > 0 on success? (point [3] above)
Yes and will document it.
>
> > + *
> > + * DNS resolution will be done synchronously so execution
> > + * of the caller may be blocked for an arbitrary length
> > + * of time.
> > + *
> > + * Returns: 0 if resolution was successful, -1 on error
> > + */
> > +int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp);
> > +
> > +/**
> > + * qio_dns_resolver_lookup_sync:
>
> s/sync/async/
>
> > + * @resolver: the DNS resolver instance
> > + * @addr: the address to resolve
> > + * @naddr: pointer to hold number of resolved addresses
> > + * @addrs: pointer to hold resolved addresses
> > + * @errp: pointer to NULL initialized error object
>
> Wrong parameters; naddr/addrs/errp should be replaced with
> func/opaque/notify.
>
> > + *
> > + * This will attempt to resolve the address provided
> > + * in @addr. The callback @func will be invoked when
> > + * resolution has either completed or failed. On
> > + * success, the @func should call the method
> > + * qio_dns_resolver_lookup_result() to obtain the
> > + * results.
> > + *
> > + * DNS resolution will be done asynchronously so execution
> > + * of the caller will not be blocked.
> > + */
> > +void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + QIOTaskFunc func,
> > + gpointer opaque,
> > + GDestroyNotify notify);
> > +
> > +/**
> > + * qio_dns_resolver_lookup_result:
> > + * @resolver: the DNS resolver instance
> > + * @task: the task object to get results for
> > + * @naddr: pointer to hold number of resolved addresses
> > + * @addrs: pointer to hold resolved addresses
> > + *
> > + * This method should be called from the callback passed
> > + * to qio_dns_resolver_lookup_async() in order to obtain
> > + * results. @addrs will be filled with all the resolved
> > + * addresses. @naddrs will specify the number of entries
> > + * allocated in @addrs. The caller is responsible for
> > + * freeing each entry in @addrs, as well as @addrs itself.
>
> Again, the free seems to be missing in the example above.
Yep, will fix.
> > +static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp)
> > +{
> > + struct addrinfo ai, *res, *e;
> > + InetSocketAddress *iaddr = addr->u.inet.data;
> > + char port[33];
> > + char uaddr[INET6_ADDRSTRLEN + 1];
> > + char uport[33];
> > + int rc;
> > + Error *err = NULL;
> > + size_t i;
> > +
> > + *naddrs = 0;
> > + *addrs = NULL;
> > +
> > + memset(&ai, 0, sizeof(ai));
> > + ai.ai_flags = AI_PASSIVE;
> > + if (iaddr->numeric) {
>
> 'iaddr->has_numeric && iaddr->numeric', unless you make sure that all
> possible initialization paths have a sane value of iaddr->numeric==false
> even when iaddr->has_numeric is false (qapi guarantees 0 initialization,
> but I'm not sure if all SocketAddress come from qapi).
Yeah, makes sense to be explicit about it anyway
> > + /* create socket + bind */
> > + for (i = 0, e = res; e != NULL; i++, e = e->ai_next) {
> > + SocketAddress *newaddr = g_new0(SocketAddress, 1);
> > + InetSocketAddress *newiaddr = g_new0(InetSocketAddress, 1);
> > + newaddr->u.inet.data = newiaddr;
> > + newaddr->type = SOCKET_ADDRESS_KIND_INET;
> > +
> > + getnameinfo((struct sockaddr *)e->ai_addr, e->ai_addrlen,
> > + uaddr, INET6_ADDRSTRLEN, uport, 32,
> > + NI_NUMERICHOST | NI_NUMERICSERV);
> > +
> > + *newiaddr = (InetSocketAddress){
> > + .host = g_strdup(uaddr),
> > + .port = g_strdup(uport),
> > + .numeric = true,
>
> Also need .has_numeric = true
>
> > + .has_to = iaddr->has_to,
> > + .to = iaddr->to,
> > + .has_ipv4 = false,
> > + .has_ipv6 = false,
> > + };
> > +
> > + (*addrs)[i] = newaddr;
> > + }
> > + freeaddrinfo(res);
> > + return 0;
> > +}
> > +
> > +
> > +static int qio_dns_resolver_lookup_sync_unix(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp)
> > +{
> > + *naddrs = 1;
> > + *addrs = g_new0(SocketAddress *, 1);
> > + (*addrs)[0] = QAPI_CLONE(SocketAddress, addr);
>
> Cool - I'm glad to see more use of my clone visitor :)
>
> > +
> > + return 0;
> > +}
> > +
> > +
> > +int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp)
> > +{
> > + switch (addr->type) {
> > + case SOCKET_ADDRESS_KIND_INET:
> > + return qio_dns_resolver_lookup_sync_inet(resolver,
> > + addr,
> > + naddrs,
> > + addrs,
> > + errp);
> > +
> > + case SOCKET_ADDRESS_KIND_UNIX:
> > + return qio_dns_resolver_lookup_sync_unix(resolver,
> > + addr,
> > + naddrs,
> > + addrs,
> > + errp);
> > +
> > + default:
>
> Do we need to play with Stefan's vsock stuff?
Oh yes, I didn't realize that was merged. It should be just
cloned as we do for unix sock.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
