Re: [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 14034 bytes --]

On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Currently DNS resolution is done automatically as part
> of the creation of a QIOChannelSocket object instance.
> This works ok for network clients where you just end
> up a single network socket, but for servers, the results
> of DNS resolution may require creation of multiple
> sockets.
> 
> Introducing a DNS resolver API allows DNS resolution
> to be separated from the socket object creation. This
> will make it practical to create multiple QIOChannelSocket
> instances for servers.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/io/dns-resolver.h | 224 +++++++++++++++++++++++++++++++++++++++
>  include/qemu/sockets.h    |   2 +
>  io/Makefile.objs          |   1 +
>  io/dns-resolver.c         | 265 ++++++++++++++++++++++++++++++++++++++++++++++
>  util/qemu-sockets.c       |   4 +-
>  5 files changed, 494 insertions(+), 2 deletions(-)
>  create mode 100644 include/io/dns-resolver.h
>  create mode 100644 io/dns-resolver.c
> 
> diff --git a/include/io/dns-resolver.h b/include/io/dns-resolver.h
> new file mode 100644
> index 0000000..5121e65
> --- /dev/null
> +++ b/include/io/dns-resolver.h
> @@ -0,0 +1,224 @@
> +/*
> + * QEMU DNS resolver
> + *
> + * Copyright (c) 2016 Red Hat, Inc.

Want to add 2017?

> +
> +/**
> + * QIODNSResolver:
> + *
> + * The QIODNSResolver class provides a framework for doing
> + * DNS resolution on SocketAddress objects, independently
> + * of socket creation.
> + *
> + * <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?

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]

> + *          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]

> + *         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.

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).

> + *            socks[nsocks++] = sock;
> + *         }
> + *      }
> + *
> + *      if (nsocks == 0) {
> + *         error_propagate(errp, err);

[3]...if the DNS lookup can succeed with nrawaddrs == 0, then you have
nsocks == 0 but no err set.  Is that a problem?  Or is nrawaddrs always
greater than 0 on success, such that nsocks == 0 always implies that we
failed to open every single socket and thus have err set?

> + *      } 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.

> + *      }
> + *   }
> + *   </programlisting>
> + * </example>
> + *
> + * <example>
> + *   <title>Resolving addresses asynchronously</title>
> + *   <programlisting>
> + *    typedef struct MyListenData {
> + *       Error *err;
> + *       QIOChannelSocket **socks;
> + *       size_t nsocks;
> + *    } MyListenData;
> + *
> + *    void mylistenresult(QIOTask *task, void *opaque) {
> + *      MyListenData *data = opaque;
> + *      QIODNSResolver *resolver =
> + *         QIO_DNS_RESOLVER(qio_task_get_source(task);
> + *      SocketAddress **rawaddrs = NULL;
> + *      size_t nrawaddrs = 0;
> + *      Error *err = NULL;
> + *
> + *      if (qio_task_propagate_error(task, &data->err)) {
> + *         return;
> + *      }
> + *
> + *      qio_dns_resolver_lookup_result(resolver, task,
> + *                                     &nrawaddrs, &rawaddrs);
> + *
> + *      for (i = 0; i < nrawaddrs; i++) {
> + *         QIOChannel *sock = qio_channel_new();
> + *         Error *local_err = NULL;
> + *         qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
> + *         if (local_err && !err) {
> + *            error_propagate(err, local_err);

Same problem as in the last example, where you don't handle
double-failure correctly, and where the code won't compile without &.

> + *         } else {
> + *            socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
> + *            socks[nsocks++] = sock;
> + *         }
> + *      }
> + *
> + *      if (nsocks == 0) {
> + *         error_propagate(&data->err, err);
> + *      } else {
> + *         error_free(err);
> + *      }
> + *    }
> + *
> + *    void mylisten(SocketAddress *addr, MyListenData *data) {
> + *      QIODNSResolver *resolver = qio_dns_resolver_get_instance();
> + *      qio_dns_resolver_lookup_async(dns, addr,
> + *                                    mylistenresult, data, NULL);
> + *    }
> + *   </programlisting>
> + * </example>
> + */

The examples are much appreciated; looking forward to v2.

> +/**
> + * 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?

Are we guaranteed that naddrs > 0 on success? (point [3] above)

> + *
> + * 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.

> + */
> +void qio_dns_resolver_lookup_result(QIODNSResolver *resolver,
> +                                    QIOTask *task,
> +                                    size_t *naddrs,
> +                                    SocketAddress ***addrs);
> +
> +#endif /* QIO_DNS_RESOLVER_H */

Overall the interface looks reasonable.


> +++ b/io/dns-resolver.c
> @@ -0,0 +1,265 @@
> +/*
> + * QEMU DNS resolver
> + *
> + * Copyright (c) 2016 Red Hat, Inc.

and 2017?

> +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).


> +    /* 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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]