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

[Dr. David Alan Gilbert]

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

> >   2) Define the 'default' macro at the top of migration.c
> >   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.

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.

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

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.

> 
> >     c) Set the value in the array if the has_ is true
> >   5) Fixup migrate_init to preserve the parameter around the init
> >   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.

> >   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,
> > 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).
> > 
> > In my ideal world there would be:
> >    a) One thing to add to qapi-schema.json
> >    b) qmp_migrate_set_parameters would take an array pointer indexed
> >       by the enum
> >    c) A way to define the bounds so that we didn't have to manually
> >       add the bound checking.
> >    d) Something where I defined the default value
> 
> 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.

Dave


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK