From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46799) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQg5U-0000ly-8J for qemu-devel@nongnu.org; Tue, 02 Feb 2016 13:54:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQg5Q-00028m-2s for qemu-devel@nongnu.org; Tue, 02 Feb 2016 13:54:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59800) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQg5P-00028h-Rj for qemu-devel@nongnu.org; Tue, 02 Feb 2016 13:54:00 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 61D63A2C08 for ; Tue, 2 Feb 2016 18:53:59 +0000 (UTC) Date: Tue, 2 Feb 2016 18:53:55 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20160202185355.GE4498@work-vm> References: <1452599056-27357-1-git-send-email-berrange@redhat.com> <1452599056-27357-13-git-send-email-berrange@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452599056-27357-13-git-send-email-berrange@redhat.com> Subject: Re: [Qemu-devel] [PATCH v1 12/22] migration: convert exec socket protocol to use QIOChannel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Amit Shah , qemu-devel@nongnu.org, Juan Quintela * 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 > --- > 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 > -#include > +#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 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