[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/3] Add migrate_incoming |
Date: |
Fri, 20 Feb 2015 14:05:29 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux) |
"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Markus Armbruster (address@hidden) 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)" <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,
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
[Qemu-devel] [PATCH v3 3/3] Document -incoming options, Dr. David Alan Gilbert (git), 2015/02/19
Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer, Michael S. Tsirkin, 2015/02/22
Re: [Qemu-devel] [PATCH v3 0/3] -incoming defer, Stefan Hajnoczi, 2015/02/23