* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Tue, Feb 02, 2016 at 06:46:01PM +0000, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berra...@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 <berra...@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 / dgilb...@redhat.com / Manchester, UK
