Re: [Qemu-devel] [PATCH 4/4] qapi: Convert migrate

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 12 Mar 2012 16:23:01 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Mon, Mar 12, 2012 at 11:01:54AM -0300, Luiz Capitulino wrote:
> > On Mon, 12 Mar 2012 15:49:27 +0200
> > Alon Levy <alevy@redhat.com> 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 <alevy@redhat.com> 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 <aliguori@us.ibm.com>
> > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > > ---
> > > > > >  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.

Oh, good idea, that's a valid workaround.

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

Will review it shortly.