[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/8] io: introduce a network socket listener API
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-block] [PATCH 2/8] io: introduce a network socket listener API |
Date: |
Fri, 11 Aug 2017 10:15:04 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
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 <address@hidden>
> > ---
>
> > +++ 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 :|
[Qemu-block] [PATCH 3/8] blockdev: convert internal NBD server to QIONetListener, Daniel P. Berrange, 2017/08/10
[Qemu-block] [PATCH 4/8] blockdev: convert qemu-nbd server to QIONetListener, Daniel P. Berrange, 2017/08/10
[Qemu-block] [PATCH 5/8] migration: convert socket server to QIONetListener, Daniel P. Berrange, 2017/08/10