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 <berra...@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
signature.asc
Description: OpenPGP digital signature
