Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming

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

* Markus Armbruster (armbru@redhat.com) wrote:
> I'd like Eric's opinion on on encoding configuration tuples as URIs
> rather than JSON in QMP.

Old discussion; he's already R-b the previous version of this
patch where the only difference was pause/defer.

Dave

> 
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Add migrate_incoming/migrate-incoming to start an incoming
> > migration.
> >
> > Once a qemu has been started with
> >     -incoming defer
> >
> > the migration can be started by issuing:
> >     migrate_incoming uri
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  hmp-commands.hx       | 16 ++++++++++++++++
> >  hmp.c                 | 14 ++++++++++++++
> >  hmp.h                 |  1 +
> >  migration/migration.c | 19 +++++++++++++++++++
> >  qapi-schema.json      | 15 +++++++++++++++
> >  qmp-commands.hx       | 31 ++++++++++++++++++++++++++++++-
> >  6 files changed, 95 insertions(+), 1 deletion(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index e37bc8b..5dcc556 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -922,6 +922,22 @@ Cancel the current VM migration.
> >  ETEXI
> >  
> >      {
> > +        .name       = "migrate_incoming",
> > +        .args_type  = "uri:s",
> > +        .params     = "uri",
> > +        .help       = "Continue an incoming migration from an -incoming defer",
> > +        .mhandler.cmd = hmp_migrate_incoming,
> > +    },
> > +
> > +STEXI
> > +@item migrate_incoming @var{uri}
> > +@findex migrate_incoming
> > +Continue an incoming migration using the @var{uri} (that has the same syntax
> > +as the -incoming option).
> > +
> > +ETEXI
> > +
> > +    {
> >          .name       = "migrate_set_cache_size",
> >          .args_type  = "value:o",
> >          .params     = "value",
> > diff --git a/hmp.c b/hmp.c
> > index b47f331..f051896 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1083,6 +1083,20 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
> >      qmp_migrate_cancel(NULL);
> >  }
> >  
> > +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    const char *uri = qdict_get_str(qdict, "uri");
> > +
> > +    qmp_migrate_incoming(uri, &err);
> > +
> > +    if (err) {
> > +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> > +        error_free(err);
> > +        return;
> > +    }
> 
> Please replace the conditional by
> 
>        hmp_handle_error(mon, err);
> 
> I'll clean up the other places that should use it.
> 
> > +}
> > +
> >  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
> >  {
> >      double value = qdict_get_double(qdict, "value");
> > diff --git a/hmp.h b/hmp.h
> > index 4bb5dca..95efe63 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -60,6 +60,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
> >  void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
> >  void hmp_drive_backup(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
> > +void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
> >  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f3d49d5..2c805f1 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -432,6 +432,25 @@ void migrate_del_blocker(Error *reason)
> >      migration_blockers = g_slist_remove(migration_blockers, reason);
> >  }
> >  
> > +void qmp_migrate_incoming(const char *uri, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    if (!deferred_incoming) {
> > +        error_setg(errp, "'-incoming defer' is required for migrate_incoming");
> > +        return;
> > +    }
> > +
> > +    qemu_start_incoming_migration(uri, &local_err);
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    deferred_incoming = false;
> > +}
> > +
> 
> The error message refers to the command as "migrate_incoming", which is
> wrong for QMP.
> 
> Apart from that, the error message is fine when we fail because the user
> didn't specify -incoming defer.  It's unhelpful when we fail because
> migrate-incoming has already been run.
> 
> You could try something like
> 
>     void qmp_migrate_incoming(const char *uri, Error **errp)
>     {
>         Error *local_err = NULL;
> 
>         if (!deferred_incoming) {
>             error_setg(errp, "'-incoming defer' is required");
>             return;
>         }
>         if (!runstate_check(RUN_STATE_INMIGRATE)) {
>             error_setg(errp, "No migration incoming"
>             return;
>         }
> 
>         qemu_start_incoming_migration(uri, &local_err);
> 
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
>         }
>     }
> 
> You're welcome to improve my rather laconic error messages.
> 
> >  void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >                   bool has_inc, bool inc, bool has_detach, bool detach,
> >                   Error **errp)
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index e16f8eb..7a80081 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1738,6 +1738,21 @@
> >  { 'command': 'migrate',
> >    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
> >  
> > +##
> > +# @migrate-incoming
> > +#
> > +# Start an incoming migration, the qemu must have been started
> > +# with -incoming defer
> > +#
> > +# @uri: The Uniform Resource Identifier identifying the source or
> > +#       address to listen on
> > +#
> > +# Returns: nothing on success
> > +#
> > +# Since: 2.3
> > +##
> > +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> > +
> >  # @xen-save-devices-state:
> >  #
> >  # Save the state of all devices to file. The RAM and the block devices
> 
> Eric, what's your take on this?
> 
> The general rule in QMP is "no ad hoc encoding of tuples in strings, use
> JSON objects instead".
> 
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index a85d847..60181c7 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -661,7 +661,36 @@ Example:
> >  <- { "return": {} }
> >  
> >  EQMP
> > -{
> > +
> > +    {
> > +        .name       = "migrate-incoming",
> > +        .args_type  = "uri:s",
> > +        .mhandler.cmd_new = qmp_marshal_input_migrate_incoming,
> > +    },
> > +
> > +SQMP
> > +migrate-incoming
> > +----------------
> > +
> > +Continue an incoming migration
> > +
> > +Arguments:
> > +
> > +- "uri": Source/listening URI (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "migrate-incoming", "arguments": { "uri": "tcp::4446" } }
> > +<- { "return": {} }
> > +
> > +Notes:
> > +
> > +(1) QEMU must be started with -incoming defer to allow migrate-incoming to
> > +    be used
> > +(2) The uri format is the same as to -incoming
> > +
> > +EQMP
> > +    {
> >          .name       = "migrate-set-cache-size",
> >          .args_type  = "value:o",
> >          .mhandler.cmd_new = qmp_marshal_input_migrate_set_cache_size,
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK