Re: [Qemu-devel] [PATCH v1 11/22] migration: convert fd socket protocol to use QIOChannel

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Tue, Feb 02, 2016 at 06:46:01PM +0000, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > Convert the fd socket migration protocol driver to use
> > > QIOChannel and QEMUFileChannel, instead of plain sockets
> > > APIs. It can be unconditionally built because the
> > > QIOChannel APIs it uses will take care to report suitable
> > > error messages if needed.
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > >  migration/Makefile.objs |  4 ++--
> > >  migration/fd.c          | 57 ++++++++++++++++++++++++++++++++-----------------
> > >  migration/migration.c   |  4 ----
> > >  3 files changed, 39 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> > > index a5f8a03..64f95cd 100644
> > > --- a/migration/Makefile.objs
> > > +++ b/migration/Makefile.objs
> > > @@ -1,11 +1,11 @@
> > > -common-obj-y += migration.o tcp.o unix.o
> > > +common-obj-y += migration.o tcp.o unix.o fd.o
> > >  common-obj-y += vmstate.o
> > >  common-obj-y += qemu-file.o qemu-file-buf.o qemu-file-unix.o qemu-file-stdio.o
> > >  common-obj-y += qemu-file-channel.o
> > >  common-obj-y += xbzrle.o postcopy-ram.o
> > >  
> > >  common-obj-$(CONFIG_RDMA) += rdma.o
> > > -common-obj-$(CONFIG_POSIX) += exec.o fd.o
> > > +common-obj-$(CONFIG_POSIX) += exec.o
> > >  
> > >  common-obj-y += block.o
> > >  
> > > diff --git a/migration/fd.c b/migration/fd.c
> > > index 3e4bed0..8d48e0d 100644
> > > --- a/migration/fd.c
> > > +++ b/migration/fd.c
> > > @@ -20,6 +20,8 @@
> > >  #include "monitor/monitor.h"
> > >  #include "migration/qemu-file.h"
> > >  #include "block/block.h"
> > > +#include "io/channel-file.h"
> > > +#include "io/channel-socket.h"
> > >  
> > >  //#define DEBUG_MIGRATION_FD
> > >  
> > > @@ -33,56 +35,71 @@
> > >  
> > >  static bool fd_is_socket(int fd)
> > >  {
> > > -    struct stat stat;
> > > -    int ret = fstat(fd, &stat);
> > > -    if (ret == -1) {
> > > -        /* When in doubt say no */
> > > -        return false;
> > > -    }
> > > -    return S_ISSOCK(stat.st_mode);
> > > +    int optval;
> > > +    socklen_t optlen;
> > > +    optlen = sizeof(optval);
> > > +    return getsockopt(fd,
> > > +                      SOL_SOCKET,
> > > +                      SO_TYPE,
> > > +                      (char *)&optval,
> > > +                      &optlen) == 0;
> > 
> > Should that be qemu_getsockopt ?
> 
> Yes.
> 
> > >  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
> > >  {
> > > +    QIOChannel *ioc;
> > >      int fd = monitor_get_fd(cur_mon, fdname, errp);
> > >      if (fd == -1) {
> > >          return;
> > >      }
> > >  
> > >      if (fd_is_socket(fd)) {
> > > -        s->file = qemu_fopen_socket(fd, "wb");
> > > +        ioc = QIO_CHANNEL(qio_channel_socket_new_fd(fd, errp));
> > > +        if (!ioc) {
> > > +            close(fd);
> > > +            return;
> > > +        }
> > 
> > Have you considered moving this fd_is_socket detection
> > glue down into channel-file.c?  It's here so that a socket
> > passed via an fd will be able to use a shutdown() method.
> > It would be nice to do the same thing to sockets in the same
> > situation in other places, so it would make sense if it worked
> > on any fd that went through channels.
> > The tricky bit is at that level you return a QIOChannelFile rather
> > than a generic QIOChannel pointer.
> > (One place I'd like to be able to a shutdown is on an nbd channel
> > for example).
> 
> Yeah, the complexity is that we have to return two different
> class instances depending on the type of FD in use. We could
> introduce some helper method todo this though.

Is there any reason they return the subclasses rather than the
common parent?

> > > -static void fd_accept_incoming_migration(void *opaque)
> > > +static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
> > > +                                             GIOCondition condition,
> > > +                                             gpointer opaque)
> > >  {
> > >      QEMUFile *f = opaque;
> > > -
> > > -    qemu_set_fd_handler(qemu_get_fd(f), NULL, NULL, NULL);
> > >      process_incoming_migration(f);
> > > +    return FALSE;
> > 
> > Please comment the magical FALSE.
> 
> FALSE is the standard value any glib event loop handled need to
> return to cause glib to unregister it. I'm not sure we really
> want to comment every event handle in this way.

I'd prefer we did.  It doesn't need to be big and detailed,
but either a function comment, or something as simple as:
    return FALSE; /* unregister */

it's not something I immediately recognised, and it took a bit of digging
around for me to find the answer.

> > >  void fd_start_incoming_migration(const char *infd, Error **errp)
> > >  {
> > > -    int fd;
> > >      QEMUFile *f;
> > > +    QIOChannel *ioc;
> > > +    int fd;
> > >  
> > >      DPRINTF("Attempting to start an incoming migration via fd\n");
> > >  
> > >      fd = strtol(infd, NULL, 0);
> > >      if (fd_is_socket(fd)) {
> > > -        f = qemu_fopen_socket(fd, "rb");
> > > +        ioc = QIO_CHANNEL(qio_channel_socket_new_fd(fd, errp));
> > > +        if (!ioc) {
> > > +            close(fd);
> > > +            return;
> > > +        }
> > 
> > Wouldn't it be better to move this check outside of this if, so that
> > you test the output of both the socket_new_fd and the file_new_fd ?
> 
> qio_channel_file_new_fd() can never fail - only the socket_new_fd can
> fail (when trying to query the sockaddr_t data).

OK, I'm not too fussed about this bit, but:
  1) It's easier to read - one level less of nesting
  2) It avoids making an assumption about qio_channel_file_new_fd() never
     failing, which is something you happen to know but you treat as
     API;  it costs nothing to avoid making that assumption.

Dave

> 
> > >      } else {
> > > -        f = qemu_fdopen(fd, "rb");
> > > -    }
> > > -    if(f == NULL) {
> > > -        error_setg_errno(errp, errno, "failed to open the source descriptor");
> > > -        return;
> > > +        ioc = QIO_CHANNEL(qio_channel_file_new_fd(fd));
> > >      }
> > > +    f = qemu_fopen_channel_input(ioc);
> > > +    object_unref(OBJECT(ioc));
> > >  
> > > -    qemu_set_fd_handler(fd, fd_accept_incoming_migration, NULL, f);
> > > +    qio_channel_add_watch(ioc,
> > > +                          G_IO_IN,
> > > +                          fd_accept_incoming_migration,
> > > +                          f,
> > > +                          NULL);
> > >  }
> 
> 
> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK