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