diff mbox

[v1,11/22] migration: convert fd socket protocol to use QIOChannel

Message ID 1452599056-27357-12-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Jan. 12, 2016, 11:44 a.m. UTC
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(-)

Comments

Dr. David Alan Gilbert Feb. 2, 2016, 6:46 p.m. UTC | #1
* 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 ?

>  }
>  
>  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).

>      } else {
> -        s->file = qemu_fdopen(fd, "wb");
> +        ioc = QIO_CHANNEL(qio_channel_file_new_fd(fd));
>      }
> +    s->file = qemu_fopen_channel_output(ioc);
>  
>      migrate_fd_connect(s);
>  }
>  
> -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.

>  }
>  
>  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 ?

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);
>  }
> diff --git a/migration/migration.c b/migration/migration.c
> index 1c5f12e..211879e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -315,10 +315,8 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>  #endif
>      } else if (strstart(uri, "unix:", &p)) {
>          unix_start_incoming_migration(p, errp);
> -#if !defined(WIN32)
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_incoming_migration(p, errp);
> -#endif
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> @@ -1022,10 +1020,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>  #endif
>      } else if (strstart(uri, "unix:", &p)) {
>          unix_start_outgoing_migration(s, p, &local_err);
> -#if !defined(WIN32)
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_outgoing_migration(s, p, &local_err);
> -#endif
>      } else {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");
> -- 
> 2.5.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Daniel P. Berrangé Feb. 3, 2016, 10:05 a.m. UTC | #2
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.


> > -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.

> >  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).

> >      } 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
Dr. David Alan Gilbert Feb. 3, 2016, 10:29 a.m. UTC | #3
* 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
Daniel P. Berrangé Feb. 3, 2016, 10:39 a.m. UTC | #4
On Wed, Feb 03, 2016 at 10:29:50AM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Tue, Feb 02, 2016 at 06:46:01PM +0000, Dr. David Alan Gilbert wrote:
> > > >  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.

Note that qio_channel_file_new_fd() does not accept a 'Error **errp' since
there is no failure condition. So if we pushed the failure check up it'd
be feel like a logic error as we'd be introducing a error return path
from the method where 'errp' may not be set.

Regards,
Daniel
diff mbox

Patch

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;
 }
 
 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;
+        }
     } else {
-        s->file = qemu_fdopen(fd, "wb");
+        ioc = QIO_CHANNEL(qio_channel_file_new_fd(fd));
     }
+    s->file = qemu_fopen_channel_output(ioc);
 
     migrate_fd_connect(s);
 }
 
-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;
 }
 
 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;
+        }
     } 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);
 }
diff --git a/migration/migration.c b/migration/migration.c
index 1c5f12e..211879e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -315,10 +315,8 @@  void qemu_start_incoming_migration(const char *uri, Error **errp)
 #endif
     } else if (strstart(uri, "unix:", &p)) {
         unix_start_incoming_migration(p, errp);
-#if !defined(WIN32)
     } else if (strstart(uri, "fd:", &p)) {
         fd_start_incoming_migration(p, errp);
-#endif
     } else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
@@ -1022,10 +1020,8 @@  void qmp_migrate(const char *uri, bool has_blk, bool blk,
 #endif
     } else if (strstart(uri, "unix:", &p)) {
         unix_start_outgoing_migration(s, p, &local_err);
-#if !defined(WIN32)
     } else if (strstart(uri, "fd:", &p)) {
         fd_start_outgoing_migration(s, p, &local_err);
-#endif
     } else {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");