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

[Markus Armbruster]

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

> * Markus Armbruster (address@hidden) wrote:
>> "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.
>
> I do like having it as an indexed array; because in principal I could
> turn some of the other stages I've mentioned into loops over the enum
> so I wouldn't have to add anything in each o fthose steps.

As long as they're all of the same type.  I guess "flags" would be, but
"parameters" could be anything, so the fact they're all boolean now
feels incidental to me.

>> 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 }
>
> Yes, it does, but the code behind it is a lot simpler.

Sometimes it takes a tough man to make a tender chicken.[*]

>> >> >   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?
>
> We've currently got three entries in the schema for each parameter; I'd
> just like one.
>
>> >> >     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.
>
> Oh that sounds promising; what type checking do I lose?.
> It sounds like it would be safer against misordering of the parameters
> in the C code.

I'm going to explain in a separate message.

>> 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' } }
>
> Except that if I have a qdict, and I have a list of names, then I can
> just loop over my array - I wouldn't have to add code to the
> migrate_set_parameters
> code for each new parameter added (although somewhere I'm going to have to
> say the type), and so the qdict_get_int is probably better.

If you have a struct and a list of member offsets, then you can just
loop over your array, too.

>> 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.
>
> Agreed, don't really want to break the external interface.

I go one step further: I don't even want to create a new messy external
interface.  Even when no old one exists.

>> 
>> >> >    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.
>
> Hmm maybe.



[*] From Gabriel's classic "Worse is Better" essay (can't resist
temptation to quote that one)
http://www.jwz.org/doc/worse-is-better.html