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

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Aug 10, 2017 at 01:12:25PM -0500, Eric Blake wrote:
> 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?

bool is fine.

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

The docs are wrong, as you noticed below

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

Yes

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

No, it should skip the add

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

Bad docs.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|