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

[Dr. David Alan Gilbert]

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

I've just posted a series which should address all the issues in
here; but note that:

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

doesn't work; because qemu_start_incoming_migration returns
immediately, and so:

migrate_incoming tcp::4444
migrate_incoming tcp::4444

fails trying to bind the port twice.  See the new patch series
for my way of fixing it.


Dave

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