"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote: > * Markus Armbruster (arm...@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.
i will send the pull request for this (actually previous version was on my list already). Please update Markus comment as a separate serie. Thanks, Juan. > > Dave > >> >> "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> writes: >> >> > From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@redhat.com> >> > Reviewed-by: Eric Blake <ebl...@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 / dgilb...@redhat.com / Manchester, UK