* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
>
> > * Eric Blake (ebl...@redhat.com) 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.
> 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.
> >> > 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.
> 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.
> 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.
>
> >> > 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.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
