Re: [Qemu-devel] [PATCH v1 RFC 23/34] io: add QIOChannelSocket class

[Daniel P. Berrange]

On Fri, Apr 17, 2015 at 05:28:09PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/04/2015 16:22, Daniel P. Berrange wrote:
> > Implement a QIOChannel subclass that supports sockets I/O
> > 
> > TBD check errno handling of windows port & fix watch impl
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  include/io/channel-socket.h | 168 +++++++++++++
> >  io/Makefile.objs            |   1 +
> >  io/channel-socket.c         | 572 
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 741 insertions(+)
> >  create mode 100644 include/io/channel-socket.h
> >  create mode 100644 io/channel-socket.c
> > 
> > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > new file mode 100644
> > index 0000000..b95349b
> > --- /dev/null
> > +++ b/include/io/channel-socket.h


> > +/**
> > + * qio_channel_socket_get_local_addr_string:
> > + * @ioc: the socket channel object
> > + * @hostname: pointer to be filled with hostname string
> > + * @servicename: pointer to be filled with servicename string
> > + * @family: pointer to be filled with network address family
> > + * @errp: pointer to an uninitialized error object
> > + *
> > + * Get the string representation of the local socket
> > + * address. The address information will be stored in
> > + * the @hostname, @servicename and @family parameters.
> > + * The @hostname and @servicename strings will be
> > + * allocated to the size required and should be free
> > + * with g_free() when no longer required
> > + *
> > + * Returns: 0 on success, -1 on error
> > + */
> > +int
> > +qio_channel_socket_get_local_addr_string(QIOChannelSocket *ioc,
> > +                                         char **hostname,
> > +                                         char **servicename,
> > +                                         NetworkAddressFamily *family,
> > +                                         Error **errp);
> > +
> > +/**
> > + * qio_channel_socket_get_remote_addr_string:
> > + * @ioc: the socket channel object
> > + * @hostname: pointer to be filled with hostname string
> > + * @servicename: pointer to be filled with servicename string
> > + * @family: pointer to be filled with network address family
> > + * @errp: pointer to an uninitialized error object
> > + *
> > + * Get the string representation of the remote socket
> > + * address. The address information will be stored in
> > + * the @hostname, @servicename and @family parameters.
> > + * The @hostname and @servicename strings will be
> > + * allocated to the size required and should be free
> > + * with g_free() when no longer required
> > + *
> > + * Returns: 0 on success, -1 on error
> > + */
> > +int
> > +qio_channel_socket_get_remote_addr_string(QIOChannelSocket *ioc,
> > +                                          char **hostname,
> > +                                          char **servicename,
> > +                                          NetworkAddressFamily *family,
> > +                                          Error **errp);
> 
> Would it be possible to change these to use a SocketAddress* type?

Yeah, that looks like it is a viable option.

> > +
> > +/**
> > + * qio_channel_socket_set_nodelay:
> > + * @ioc: the socket channel object
> > + * @enabled: the new flag state
> > + *
> > + * Set the state of the NODELAY socket flag. If the
> 
> The function name is okay, but please write TCP_NODELAY in the
> documentation, or talk about Nagle's algorithm instead of mentioning the
> flag.

Sure will do.

> > + * @enabled parameter is true, then NODELAY will be
> > + * set and data will be transmitted immediately. If
> > + * @enabled is false, then data may be temporarily
> > + * held for transmission to enable writes to be
> > + * coallesced.
> > + */
> > +void
> > +qio_channel_socket_set_nodelay(QIOChannelSocket *ioc,
> > +                               bool enabled);
> > +
> > +/**
> > + * qio_channel_socket_accept:
> > + * @ioc: the socket channel object
> > + * @errp: pointer to an uninitialized error object
> > + *
> > + * If the socket represents a server, then this accepts
> > + * a new client connection. The returned channel will
> > + * represent the connected client socket.
> > + *
> > + * Returns: the new client channel, or NULL on error
> > + */
> > +QIOChannelSocket *
> > +qio_channel_socket_accept(QIOChannelSocket *ioc,
> > +                          Error **errp);
> 
> Does it make sense for a passive socket to be a QIOChannelSocket?  We
> have already a pretty decent API in util/qemu-sockets.c, and
> QIOChannelSocket will become more similar to qemu-sockets if you switch
> to SocketAddress.  Perhaps this function can just take a file descriptor?

I was somewhat undecided about that really - One of my todos is to see
about better integrating with qemu-sockets for the connection facilities,
so will consider this problem too.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|