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

[Markus Armbruster]

I'd like Eric's opinion on on encoding configuration tuples as URIs
rather than JSON in QMP.

"Dr. David Alan Gilbert (git)" <address@hidden> writes:

> From: "Dr. David Alan Gilbert" <address@hidden>
>
> 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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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
> address@hidden migrate_incoming @var{uri}
> address@hidden 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,