Re: [Qemu-devel] [PATCH 2/8] io: introduce a network socket listener API

[Eric Blake <eblake@redhat.com>]

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

On 08/10/2017 11:04 AM, Daniel P. Berrange wrote:
> The existing QIOChannelSocket class provides the ability to
> listen on a single socket at a time. This patch introduces
> a QIONetListener class that provides a higher level API
> concept around listening for network services, allowing
> for listening on multiple sockets.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

> +++ b/include/io/net-listener.h
> @@ -0,0 +1,174 @@
> +/*
> + * QEMU I/O network listener
> + *
> + * Copyright (c) 2016 Red Hat, Inc.

Want to add 2017?

At least it's covered by MAINTAINERS :)


> +/**
> + * qio_net_listener_is_disconnected:
> + * @listener: the network listener object
> + *
> + * Determine if the listener is connected to any socket
> + * channels
> + *
> + * Returns: TRUE if connected, FALSE otherwise
> + */
> +gboolean qio_net_listener_is_disconnected(QIONetListener *listener);
> +

Must it return gboolean, or is bool sufficient?

TRUE if connected for a function named 'is_disconnected' sounds
backwards.  Avoid the double negative, name it:

qio_net_listener_is_connected(), returning true if connected

> +++ b/io/net-listener.c
> @@ -0,0 +1,315 @@
> +/*
> + * QEMU network listener
> + *
> + * Copyright (c) 2016 Red Hat, Inc.

More 2017.  Probably for the whole series :)


> +static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
> +                                              GIOCondition condition,
> +                                              gpointer opaque)
> +{

Again, can we use bool instead of gboolean?

> +int qio_net_listener_open_sync(QIONetListener *listener,
> +                               SocketAddress *addr,
> +                               Error **errp)
> +{
> +    QIODNSResolver *resolver = qio_dns_resolver_get_instance();
> +    SocketAddress **resaddrs;
> +    size_t nresaddrs;
> +    size_t i;
> +    Error *err = NULL;
> +    bool success = false;
> +
> +    if (qio_dns_resolver_lookup_sync(resolver,
> +                                     addr,
> +                                     &nresaddrs,
> +                                     &resaddrs,
> +                                     errp) < 0) {
> +        return -1;
> +    }
> +
> +    for (i = 0; i < nresaddrs; i++) {
> +        QIOChannelSocket *sioc = qio_channel_socket_new();
> +
> +        if (qio_channel_socket_listen_sync(sioc, resaddrs[i],
> +                                           err ? NULL : &err) == 0) {
> +            success = true;
> +        }

This says that as long as at least one address connected, we are
successful...

> +
> +        qio_net_listener_add(listener, sioc);

...but this adds sioc as a listener regardless of whether listen_sync()
succeeded.  Is that right?


> +gboolean qio_net_listener_is_disconnected(QIONetListener *listener)
> +{
> +    return listener->disconnected;

Documentation says it returns true on connected, but here you are
returning true on disconnected?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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