From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S769e-0003lr-5Q for qemu-devel@nongnu.org; Mon, 12 Mar 2012 10:23:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S769X-0001jm-4H for qemu-devel@nongnu.org; Mon, 12 Mar 2012 10:23:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S769W-0001jR-T7 for qemu-devel@nongnu.org; Mon, 12 Mar 2012 10:23:11 -0400 Date: Mon, 12 Mar 2012 16:23:01 +0200 From: Alon Levy Message-ID: <20120312142301.GN6256@garlic> References: <1331316786-7752-1-git-send-email-lcapitulino@redhat.com> <1331316786-7752-5-git-send-email-lcapitulino@redhat.com> <20120309225129.GT7273@garlic.redhat.com> <20120312102120.40cf18bf@doriath.home> <20120312134927.GM6256@garlic> <20120312110154.69e24e09@doriath.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120312110154.69e24e09@doriath.home> Subject: Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, jan.kiszka@siemens.com On Mon, Mar 12, 2012 at 11:01:54AM -0300, Luiz Capitulino wrote: > On Mon, 12 Mar 2012 15:49:27 +0200 > Alon Levy wrote: > > > On Mon, Mar 12, 2012 at 10:21:20AM -0300, Luiz Capitulino wrote: > > > On Sat, 10 Mar 2012 00:51:29 +0200 > > > Alon Levy wrote: > > > > > > > On Fri, Mar 09, 2012 at 03:13:06PM -0300, Luiz Capitulino wrote: > > > > > The migrate command is one of those commands where HMP and QMP completely > > > > > mix up together. This made the conversion to the QAPI (which separates the > > > > > command into QMP and HMP parts) a bit difficult. > > > > > > > > > > The first important change to be noticed is that this commit completes the > > > > > removal of the Monitor object from migration code, started by the previous > > > > > commit. > > > > > > > > > > Another important and tricky change is about supporting the non-detached > > > > > mode. That's, if the user doesn't pass '-d' the migrate command will lock > > > > > the monitor and will only release it when migration is finished. > > > > > > > > > > To support that in the new HMP command (hmp_migrate()), it is necessary > > > > > to create a timer which runs every second and checks if the migration is > > > > > still active. If it's, the timer callback will re-schedule itself to run > > > > > one second in the future. If the migration has already finished, the > > > > > monitor lock is relased and the user can use it normally. > > > > > > > > > > All these changes should be transparent to the user. > > > > > > > > > > Signed-off-by: Anthony Liguori > > > > > Signed-off-by: Luiz Capitulino > > > > > --- > > > > > hmp-commands.hx | 3 +-- > > > > > hmp.c | 56 +++++++++++++++++++++++++++++++++++++++++++++ > > > > > hmp.h | 1 + > > > > > migration-fd.c | 2 +- > > > > > migration.c | 66 ++++++++++++++---------------------------------------- > > > > > migration.h | 3 --- > > > > > qapi-schema.json | 21 +++++++++++++++++ > > > > > qmp-commands.hx | 9 +------- > > > > > savevm.c | 13 +++++------ > > > > > sysemu.h | 2 +- > > > > > 10 files changed, 105 insertions(+), 71 deletions(-) > > > > > > > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > > > > index ed88877..a86f53f 100644 > > > > > --- a/hmp-commands.hx > > > > > +++ b/hmp-commands.hx > > > > > @@ -806,8 +806,7 @@ ETEXI > > > > > " full copy of disk\n\t\t\t -i for migration without " > > > > > "shared storage with incremental copy of disk " > > > > > "(base image shared between src and destination)", > > > > > - .user_print = monitor_user_noop, > > > > > - .mhandler.cmd_new = do_migrate, > > > > > + .mhandler.cmd = hmp_migrate, > > > > > }, > > > > > > > > > > > > > > > diff --git a/hmp.c b/hmp.c > > > > > index 3a54455..0f62b3f 100644 > > > > > --- a/hmp.c > > > > > +++ b/hmp.c > > > > > @@ -14,6 +14,7 @@ > > > > > */ > > > > > > > > > > #include "hmp.h" > > > > > +#include "qemu-timer.h" > > > > > #include "qmp-commands.h" > > > > > > > > > > static void hmp_handle_error(Monitor *mon, Error **errp) > > > > > @@ -856,3 +857,58 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict) > > > > > > > > > > hmp_handle_error(mon, &error); > > > > > } > > > > > + > > > > > +typedef struct MigrationStatus > > > > > +{ > > > > > + QEMUTimer *timer; > > > > > + Monitor *mon; > > > > > +} MigrationStatus; > > > > > + > > > > > +static void hmp_migrate_status_cb(void *opaque) > > > > > +{ > > > > > + MigrationStatus *status = opaque; > > > > > + MigrationInfo *info; > > > > > + > > > > > + info = qmp_query_migrate(NULL); > > > > > + if (!info->has_status || strcmp(info->status, "active") == 0) { > > > > > + qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) + 1000); > > > > > + } else { > > > > > + monitor_resume(status->mon); > > > > > + qemu_del_timer(status->timer); > > > > > + g_free(status); > > > > > + } > > > > > + > > > > > + qapi_free_MigrationInfo(info); > > > > > +} > > > > > + > > > > > +void hmp_migrate(Monitor *mon, const QDict *qdict) > > > > > +{ > > > > > + int detach = qdict_get_try_bool(qdict, "detach", 0); > > > > > + int blk = qdict_get_try_bool(qdict, "blk", 0); > > > > > + int inc = qdict_get_try_bool(qdict, "inc", 0); > > > > > + const char *uri = qdict_get_str(qdict, "uri"); > > > > > + Error *err = NULL; > > > > > + > > > > > + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err); > > > > > + if (err) { > > > > > + monitor_printf(mon, "migrate: %s\n", error_get_pretty(err)); > > > > > + error_free(err); > > > > > + return; > > > > > + } > > > > > + > > > > > + if (!detach) { > > > > > + MigrationStatus *status; > > > > > + > > > > > + if (monitor_suspend(mon) < 0) { > > > > > > > > So using monitor_suspend is ok? is that a possible solution for the > > > > screendump issue? I could pass the Monitor through the vga callback. Tbh > > > > I think it's ugly, but at least it doesn't introduce any new command. > > > > > > This is only valid for HMP. > > > > > > > Yes, I'm thinking at this point for qmp to wait for the output file to > > be written via repeated poll / inotify. > > Making this synchronous you mean? This will hold the global mutex for too > long, won't it? No, not at all what I mean. I'm talking about a libvirt change. There is nothing that we can do in qemu other than maybe implementing a qmp_suspend (automatic nack), already suggested screendump-async (nack by no update) and now waiting for comments on just the HMP fix for screendump.