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

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

* Daniel P. Berrange (berrange@redhat.com) wrote:
> Convert the exec socket migration protocol driver to use
> QIOChannel and QEMUFileChannel, instead of the stdio
> popen APIs. It can be unconditionally built because the
> QIOChannelCommand class can report suitable error messages
> on platforms which can't fork processes.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  migration/Makefile.objs |  3 +--
>  migration/exec.c        | 48 ++++++++++++++++++++++++++++++------------------
>  migration/migration.c   |  4 ----
>  3 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> index 64f95cd..3c90c44 100644
> --- a/migration/Makefile.objs
> +++ b/migration/Makefile.objs
> @@ -1,11 +1,10 @@
> -common-obj-y += migration.o tcp.o unix.o fd.o
> +common-obj-y += migration.o tcp.o unix.o fd.o exec.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
>  
>  common-obj-y += block.o
>  
> diff --git a/migration/exec.c b/migration/exec.c
> index 8406d2b..6159aba 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -15,14 +15,8 @@
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
>  
> -#include "qemu-common.h"
> -#include "qemu/sockets.h"
> -#include "qemu/main-loop.h"
>  #include "migration/migration.h"
> -#include "migration/qemu-file.h"
> -#include "block/block.h"
> -#include <sys/types.h>
> -#include <sys/wait.h>
> +#include "io/channel-command.h"
>  
>  //#define DEBUG_MIGRATION_EXEC
>  
> @@ -36,34 +30,52 @@
>  
>  void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>  {
> -    s->file = qemu_popen_cmd(command, "w");
> -    if (s->file == NULL) {
> -        error_setg_errno(errp, errno, "failed to popen the migration target");
> +    QIOChannel *ioc;
> +    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> +
> +    DPRINTF("Attempting to start an outgoing migration\n");

No new DPRINTF's - trace_ please.

> +    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> +                                                    O_WRONLY,
> +                                                    errp));
> +    if (!ioc) {
>          return;
>      }
>  
> +    s->file = qemu_fopen_channel_output(ioc);
> +    object_unref(OBJECT(ioc));
> +
>      migrate_fd_connect(s);
>  }
>  
> -static void exec_accept_incoming_migration(void *opaque)
> +static gboolean exec_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;

As previous patch, comment the magical FALSE.

Other than those two minor ones;

(If we're lucky this might fix the rcu race that I can trigger with exec: migration).

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

>  }
>  
>  void exec_start_incoming_migration(const char *command, Error **errp)
>  {
>      QEMUFile *f;
> +    QIOChannel *ioc;
> +    const char *argv[] = { "/bin/sh", "-c", command, NULL };
>  
>      DPRINTF("Attempting to start an incoming migration\n");
> -    f = qemu_popen_cmd(command, "r");
> -    if(f == NULL) {
> -        error_setg_errno(errp, errno, "failed to popen the migration source");
> +    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> +                                                    O_RDONLY,
> +                                                    errp));
> +    if (!ioc) {
>          return;
>      }
>  
> -    qemu_set_fd_handler(qemu_get_fd(f), exec_accept_incoming_migration, NULL,
> -                        f);
> +    f = qemu_fopen_channel_input(ioc);
> +    object_unref(OBJECT(ioc));
> +
> +    qio_channel_add_watch(ioc,
> +                          G_IO_IN,
> +                          exec_accept_incoming_migration,
> +                          f,
> +                          NULL);
>  }
> diff --git a/migration/migration.c b/migration/migration.c
> index 211879e..2d2079d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -309,10 +309,8 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>      } else if (strstart(uri, "rdma:", &p)) {
>          rdma_start_incoming_migration(p, errp);
>  #endif
> -#if !defined(WIN32)
>      } else if (strstart(uri, "exec:", &p)) {
>          exec_start_incoming_migration(p, errp);
> -#endif
>      } else if (strstart(uri, "unix:", &p)) {
>          unix_start_incoming_migration(p, errp);
>      } else if (strstart(uri, "fd:", &p)) {
> @@ -1014,10 +1012,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      } else if (strstart(uri, "rdma:", &p)) {
>          rdma_start_outgoing_migration(s, p, &local_err);
>  #endif
> -#if !defined(WIN32)
>      } else if (strstart(uri, "exec:", &p)) {
>          exec_start_outgoing_migration(s, p, &local_err);
> -#endif
>      } else if (strstart(uri, "unix:", &p)) {
>          unix_start_outgoing_migration(s, p, &local_err);
>      } else if (strstart(uri, "fd:", &p)) {
> -- 
> 2.5.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK