Re: [Qemu-devel] Adding new migration-parameters - any easier way?

[Markus Armbruster]

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

> * Eric Blake (address@hidden) wrote:
>> On 06/05/2015 03:50 AM, Dr. David Alan Gilbert wrote:
>> > Hi,
>> >   Is there any way that we could make it easier to add new migration
>> > parameters? The current way is complicated and error prone;
>> > as far as I can tell, to add a new parameter we need to:
>> > 
>> >   1) qapi-schema.json
>> >     a) Add to 'MigrationParameter' enum, include comment
>> >     b) Add to migrate-set-parameters
>> >     c) Add to MigrationParameters
>> 
>> Perhaps putting a "see XXX for use" could reduce documentation
>> duplication, but it doesn't help the need to add to the enum and to
>> multiple clients.
>
> That's a shame; the MigrationCapability equivalent is so much simpler.

Relations between the three:

* enum MigrationParameter enumerates the members of struct
  MigrationParameters

* command migrate-set-parameters has the members of struct
  MigrationParameters as parameters, except the parameters are all
  optional.

I can't see what enum MigrationParameter is good for.  Commit 43c60a8
looks misguided to me: MigrationState member parameters[] is always used
element-wise, never as a whole.  Reverting that part should get rid of
the enum.

If the duplication between parameters and struct bothers us, we could
eliminate it: use the struct both as parameter of migrate-set-parameters
(requires making all struct members optional), and as value of
query-migrate-parameters (with a comment that the members are always
present).  However, see 4).

I dislike MigrationCapability, because

    [ {"state": false, "capability": "xbzrle"},
      {"state": false, "capability": "rdma-pin-all"},
      {"state": false, "capability": "auto-converge"},
      {"state": false, "capability": "zero-blocks"},
      {"state": false, "capability": "compress"} ]

feels stilted compared to the straightforward

    { "xbzrle": false,
      "rdma-pin-all": false,
      "auto-converge": false,
      "zero-blocks": false,
      "compress": false }

>> >   2) Define the 'default' macro at the top of migration.c

Used in exactly one place.  The notiational overhead is self-inflicted,
I'm afraid :)

>> >   3) Add the initialisation to migrate_get_current to set the default
>> 
>> If we get to the point where qapi can define default values for
>> variables, then the defaulting moves out of migration.c into the .json file.

Define "default value".

We discussed a default feature for a command's data, where the
definition is obvious.

What would it mean for a member of a command's returns or an event's
data to have a default?  What does it mean for a struct member (that may
or may not be used as any command's or event's data or returns)?

Here, we're talking about the initial value of
current_migration.parameters[], not some QMP command's arguments.  How
is that connected to any of the possible QAPI default features above?

See also 4a).

> Yes, that would be good.
>
>> >   4) qmp_migrate_set_parameters:
>> >     a) Add the 'has' and value arguments to qmp_migrate_set_parameters
>> >        *** Make really sure this matches the order in 
>> > migrate-set-parameters!
>> 
>> Also, moving defaults into qapi will eliminate the need for the has_
>> counterpart on optional variables (the C code will be passed the
>> defaulted value, if the user omitted the variable at the QMP layer).
>
> No, that doesn't work.  Any one call to qmp_migrate_set_parameters might
> only be changing one or a subset of the parameters; you don't want the
> rest of them to get set back to the default values.

Yes.  The default for these command arguments isn't a value to set, it's
"don't change the current setting".

>> >     b) Add a bounds check on the value
>> 
>> Once we have the qapi syntax for defaults, it would not be that much
>> more work to move bounds checking into qapi.  For example:
>> 
>> 'data': { 'value': { 'type': 'uint', 'default': 1, 'max': 10 } }
>> 
>> would be a reasonable way to document an option that can range from 0 to
>> 10 but defaults to 1.

I guess we could add a bounds feature to QAPI one way or the other.
Whether it's worth the extra complexity depends on how widely and
profitably it could be used.

> Yes, that would be nice - it's a pity we can't take that 'data' definition
> and use it for all the three uses; that ensures they're all consistent.

You lost me.  Which three uses?

>> >     c) Set the value in the array if the has_ is true

I guess you don't mind this step.

>> >   5) Fixup migrate_init to preserve the parameter around the init

Necessary only because the struct mixes up transient and permanent
stuff.  The transient stuff needs to be zeroed, while the permanent must
not be changed.  You save away the permanent stuff, zero everything,
then restore the permanent stuff.  Separate the two, and this bit of
pain should go away.

>> >   6) Add a bool and case entry to hmp_migrate_set_parameter and
>> >     pass to qmp_migrate_set_parameters
>> >        *** Make sure you get the order to qmp_migrate_set_parameters right
>> 
>> Is there a way to pass a QDict instead of individual parameters to make
>> this part easier?  Back when we started adding blockdev-add, a lot of
>> the magic was related to adding code for passing dictionaries around
>> (keeping things in name/value pairs through more of the call stack)
>> rather than adding parameters right and left at all points.
>
> Yes, a QDict like the options would be much easier.

The arguments get parsed into a QDict.  The generated command
unmarshaller checks the QDict, and calls the C function the QAPI command
definition implies with arguments extracted from the QDict.

You can bypass the generated unmarshaller: add "'gen': false" to the
schema, and make .mhandler.cmd_new point to your own unmarshaller in
qmp-commands.hx.  However, this is likely to bypass more type checking
than you want bypassed.

We could add a way to request an unmarshaller that passes some or all
arguments in a QDict rather than as single arguments.

But I doubt a QDict is really what you need here.  Wouldn't a C struct
with suitable members be a nicer parameter?  What would you rather have:

    param->compress_level                  /* checked at compile-time */
    qdict_get_int(param, "compress_level") /* mostly at run-time */

No generator hackery required,

    { 'command': 'migrate-set-parameters',
      'data': { 'param: 'MigrationParameters' } }

should do.  Except for two issues:

* You either have to make the members of MigrationParameters optional,
  or use a new type just like MigrationParameters except the members are
  optional.

* ABI break: requires an extra pair of curlies on the wire.  Possible
  solutions:

  - Add a generator feature to get rid of them (like we did with flat
    unions)

  - Deprecate the command and start over.

>> >   7) Fixup hmp_info_migrate_parameters
>> 
>> > 
>> > oh, and don't forget to:
>> >   8) add the entries to qmp_query_migrate_parameters
>> > 
>> > (I forgot).
>> 
>> Yeah, that's a lot to do.  I'm not sure if there is anything else that
>> can be done to make it more automatic in some of those places, but even
>> having a list of things to touch helps future additions.  Maybe worth
>> something in docs/?
>> 
>> > 
>> > 
>> > The three separate changes needed in the qapi-schema.json seem odd,

As discussed above, one of the three (enum) looks basically stupid to
me.  The other two (command argument, query returns) are a somewhat
common pattern.  Unifying them should be possible, if you can accept the
optionalness trouble.

>> > and the 'has'/value pairs on qmp_migrate_set_parameters is just a nightmare
>> > because there's nothing to check the ordering, and it's just getting
>> > a silly number of arguments to the function now (I've got 10
>> > parameters in one of my dev worlds, so that function has 21 arguments).

Hardly a pretty sight :)

What about wrapping them all in a C struct and passing that?

>> > In my ideal world there would be:
>> >    a) One thing to add to qapi-schema.json

Or at least fewer things.

>> >    b) qmp_migrate_set_parameters would take an array pointer indexed
>> >       by the enum

As discussed above, this messes up the external interface.  I'm rather
unwilling to accept that just to make our code a bit easier to maintain.

>> >    c) A way to define the bounds so that we didn't have to manually
>> >       add the bound checking.

If bounds checking is sufficiently common, we can try to move it into
the generated code.

>> >    d) Something where I defined the default value

"Something"?  And why would that be nicer than migration.c?

If you're asking for a way to define in the QAPI schema: where would it
go?  The schema doesn't define MigrationState.parameters[], let alone
its default value.

If MigrationState.parameters[] was replaced by something defined in the
schema, perhaps a "something" would emerge.

>> Not sure I can simplify a) or b); but c) and d) seem doable at the qapi
>> level.
>
> Well, that would be better; and the qdict for (b) that you suggest would
> also get rid of the silly number of parameters.

Hope this helps at least a little.