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

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

[Dr. David Alan Gilbert]

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
  2) Define the 'default' macro at the top of migration.c
  3) Add the initialisation to migrate_get_current to set the default
  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!
    b) Add a bounds check on the value
    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
  7) Fixup hmp_info_migrate_parameters


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

(I'm fairly sure earlier versions of migrate parameters patches
managed (a) and possibly (b)).

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) 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
>   2) Define the 'default' macro at the top of migration.c
>   3) Add the initialisation to migrate_get_current to set the default
>   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!
>     b) Add a bounds check on the value
>     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
>   7) Fixup hmp_info_migrate_parameters

oh, and don't forget to:
  8) add the entries to qmp_query_migrate_parameters

(I forgot).

Dave
> 
> 
> 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
> 
> (I'm fairly sure earlier versions of migrate parameters patches
> managed (a) and possibly (b)).
> 
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3557 bytes --]

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.

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

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

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

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Dr. David Alan Gilbert]

* Eric Blake (eblake@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.

> >   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 / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Eric Blake (eblake@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.

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.

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

[Dr. David Alan Gilbert]

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

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Eric Blake (eblake@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.

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

[Qemu-devel] qapi: What does "'gen': false" actually do, and when should I use it? (was: Adding new migration-parameters - any easier way?)

[Markus Armbruster]

Two parts: 1. what does it do, and 2. when to use it.


Part 1: What does it do?

qapi-code-gen.txt explains it thus:

    In rare cases, QAPI cannot express a type-safe representation of a
    corresponding Client JSON Protocol command.  In these cases, if the
    command expression includes the key 'gen' with boolean value false,
    then the 'data' or 'returns' member that intends to bypass generated
    type-safety and do its own manual validation should use an inline
    dictionary definition, with a value of '**' rather than a valid type
    name for the keys that the generated code will not validate.  Please
    try to avoid adding new commands that rely on this, and instead use
    type-safe unions.  For an example of bypass usage:

     { 'command': 'netdev_add',
       'data': {'type': 'str', 'id': 'str', '*props': '**'},
       'gen': false }

I'm afraid that doesn't really answer the "what does it actually do?"
question, so let's examine an example.  netdev_add isn't the best choice
because it doesn't return anything, so let's use qom-get and qom-list.

{ 'command': 'qom-list',
  'data': { 'path': 'str' },
  'returns': [ 'ObjectPropertyInfo' ] }

{ 'command': 'qom-get',
  'data': { 'path': 'str', 'property': 'str' },
  'returns': '**',
  'gen': false }

qapi-commands.py generates qmp-marshal.c and qmp-commands.h.  For
qom-list, it generates qmp_marshal_input_qom_list().  For qom-get, it
generates nothing.

We still rely on qmp-commands.hx to bind the monitor to the schema.  For
qom-list, it binds to the generated function:

    {
        .name       = "qom-list",
        .args_type  = "path:s",
        .mhandler.cmd_new = qmp_marshal_input_qom_list,
    },

For qom-get, it binds to a hand-written one:

{
        .name       = "qom-get",
	.args_type  = "path:s,property:s",
	.mhandler.cmd_new = qmp_qom_get,
    },

Both are QMP command handlers, i.e their type is

    void (*cmd_new)(QDict *params, QObject **ret_data, Error **errp);

What does the generated handler do for us?  As the filename
qmp-marshal.c suggests, it's basically a wrapper around a command
handler with a "native C" interface that unmarshals from QDict to C
"native C" data types and back.  For qom-list, the native handler is

    ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
    {
        // Do stuff with the arguments, create the result
        return props;
    }

This is the part you need to implement manually.

For qom-get, you implement this one instead:

    int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
    {
        // Unmarshall qdict into arguments
        const char *path = qdict_get_str(qdict, "path");
        const char *property = qdict_get_str(qdict, "property");
        Error *local_err = NULL;
        Object *obj;

        // Do stuff with the arguments, create the result

        // Marshal result into QObject
        // nothing to do in this case, because it happens to be one
        // already

    out:
        if (local_err) {
            // Go from Error API to QMP's QError
            // (QError will go away soon)
            qerror_report_err(local_err);
            error_free(local_err);
            return -1;
        }

        return 0;
    }

Note the boilerplate around 'do stuff'.

Notably absent from the boilerplate: rejecting unknown parameters,
checking mandatory parameters are present, checking types.  How come?

For historical reasons, part of the unmarshalling job is done by QMP
core, using args_type information from qmp-commands.hx.

For qom-list, .args_type = "path:s".  The core checks argument "path" is
present and is a JSON string, and no other argument is present.

For qom-get, .args_type = "path:s,property:s".  I trust you can guess
what the core checks there.

When we eliminate qmp-commands.hx, the core's checking goes away, and
needs to be replaced.

Generated handlers like qmp_marshal_input_qom_list() already do full
type checking of known parameters, so we just rely on that.  Handwritten
ones like qmp_qom_get() generally don't.  They'll need to be updated to
do it.


Part 2: When to use it?

We use 'gen': false when we can't (be bothered to) specify the exact
type of an argument or result.

Bad example: netdev_add

    We have arguments 'type': 'str' and '*props': '**'.

    We should have a union tagged by network backend type.  For each
    type, the union holds the type's properties (if any).

Better example: device_add (but that's not even QAPIfied, yet)

    If QAPIfied, we'd have arguments like 'driver': 'str' and '*props':
    '**'.

    Looks just like netdev_add.  The difference is that network backends
    and their properties are defined in one place, but device models and
    their properties aren't.  They get collected at run time.  As long
    as the schema is fixed at compile-time, it can't express the
    resulting tagged union.

Another good example: qom-get

    We have a return value '**'.

    The run time type is the type of the property identified by the
    arguments.  Therefore, the compile time type can only be the union
    of all property types, which effectively boils down to "anything".

    The only way to say "anything" right now is '**'.  Requires 'gen':
    false.  I figure we could extend the generators to support '**' in a
    few places, which may let us avoid 'gen': false here.

Drawback of '**': introspection knows nothing.

Introspection knowing nothing about netdev_add's and device_add's
acceptable properties is a big, painful gap.

Please don't invent new reasons for 'gen': false without a very
compelling use case.  If you think you have one, we need to talk to make
sure use of 'gen': false really beats the alternatives.  Alternatives
may include extending the generators.

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

[zhanghailiang]

Hi,

Is there any news about this discussion?
Is anyone working on it? ;)

Since the 'hard feature freeze' time is closer, we'd better to fix it in 2.4
before libvirt uses it.

I have sent a RFC patch "[RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension"
http://patchwork.ozlabs.org/patch/482125/
Which just changes qmp command to
  '{ "execute": "migrate-set-parameters" , "arguments":
      { "compression": { "compress-level": 1 } } }',
Compared with its original way, it seems to be more easy for reading and extension, but
it can't address most problems discussed here...

Thanks,
zhanghailiang

On 2015/6/5 17:50, 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
>    2) Define the 'default' macro at the top of migration.c
>    3) Add the initialisation to migrate_get_current to set the default
>    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!
>      b) Add a bounds check on the value
>      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
>    7) Fixup hmp_info_migrate_parameters
>
>
> 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
>
> (I'm fairly sure earlier versions of migrate parameters patches
> managed (a) and possibly (b)).
>
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
>

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

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> Hi,
>
> Is there any news about this discussion?
> Is anyone working on it? ;)
>
> Since the 'hard feature freeze' time is closer, we'd better to fix it in 2.4
> before libvirt uses it.
>
> I have sent a RFC patch "[RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension"
> http://patchwork.ozlabs.org/patch/482125/
> Which just changes qmp command to
>  '{ "execute": "migrate-set-parameters" , "arguments":
>      { "compression": { "compress-level": 1 } } }',
> Compared with its original way, it seems to be more easy for reading and extension, but
> it can't address most problems discussed here...

>From a QMP wire protocol point of view, @migrate-set-parameters is just
fine as it is:

    #
    # @migrate-set-parameters
    #
    # Set the following migration parameters
    #
    # @compress-level: compression level
    #
    # @compress-threads: compression thread count
    #
    # @decompress-threads: decompression thread count
    #
    # Since: 2.4
    ##
    { 'command': 'migrate-set-parameters',
      'data': { '*compress-level': 'int',
                '*compress-threads': 'int',
                '*decompress-threads': 'int'} }

However, as the number of parameters grows, the generated C interface
becomes more and more unwieldy:

    void qmp_migrate_set_parameters(bool has_compress_level,
                                    int64_t compress_level,
                                    bool has_compress_threads,
                                    int64_t compress_threads,
                                    bool has_decompress_threads,
                                    int64_t decompress_threads,
                                    ... more ...
                                    Error **errp)

Your [RFC PATCH] hides away some of the parameters in a separate
MigrationCompressParameter (misnamed, should be plural).  Leads to

    void qmp_migrate_set_parameters(bool has_compress,
                                    MigrationCompressParameters *compress,
                                    ... more ...
                                    Error **errp)

Makes the QMP wire protocol more complex, because now you have to
collect some parameters in extra curlies.

Note the two levels of optionalness: one for the wrapping struct, one
for each of its members.  If you made the members non-optional, we'd
have to specify none or all, which is not what we want.  If you made the
wrapping struct non-optional, you'd have to add a stupid 'compress': {}
when you want to set something else.  So we need both.

In C, the relatively simple test has_compress_level becomes compress &&
compress->has_level.

If we ever want to set migration parameters from the command line, the
nesting will be in the way.  Our tool to parse command line into
QAPI-generated data types (OptsVisitor) by design doesn't cope with
nested structs.

Perhaps the extra complexity is worthwhile, but it's certainly not
obvious.

If all we want is making adding new parameters easier (this thread's
stated subject), then I guess having the function take a single struct
parameter for all its arguments would do:

    void qmp_migrate_set_parameters(MigrationParameters *parms,
                                    Error **errp)

To get that from the current QAPI code generator, we'd have to do
something like

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

Trouble is this messes up the QMP wire interface: you now have to wrap
the actual arguments in two curlies instead of one.

Not a backward-compatibility issue, because the command is new.

Ugly all the same.  

To avoid the ugliness, we could change the QAPI generator.  Currently,

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

generates the same interface as when you inline MigrationParameters,
namely

    void qmp_migrate_set_parameters(bool has_compress_level,
                                    int64_t compress_level,
                                    bool has_compress_threads,
                                    int64_t compress_threads,
                                    bool has_decompress_threads,
                                    int64_t decompress_threads,
                                    ... more ...
                                    Error **errp)

It could instead generate

    void qmp_migrate_set_parameters(MigrationParameters *parms,
                                    Error **errp)

No change to the wire protocol.  Fairly big, but relatively mechanical
change to the handler functions.  I'd be willing to give it a shot and
see how it turns out, but I can't do it for 2.4, sorry.

Re: [Qemu-devel] qapi: What does "'gen': false" actually do, and when should I use it?

[Paolo Bonzini]

On 09/06/2015 10:42, Markus Armbruster wrote:
> Part 2: When to use it?
> 
> We use 'gen': false when we can't (be bothered to) specify the exact
> type of an argument or result.
> 
> Bad example: netdev_add
> 
>     We have arguments 'type': 'str' and '*props': '**'.
> 
>     We should have a union tagged by network backend type.  For each
>     type, the union holds the type's properties (if any).

The problem with this is that netdev_add was not type safe, because it
uses qemu_opts_from_qdict and QemuOpts is exclusively string-based.  So
you could write 'port': '123' or 'port': 123, and both would work, the
conversion to integer is done by the QemuOptsVisitor.

Note that device_add would have the same problem.

Paolo

> Better example: device_add (but that's not even QAPIfied, yet)
> 
>     If QAPIfied, we'd have arguments like 'driver': 'str' and '*props':
>     '**'.
> 
>     Looks just like netdev_add.  The difference is that network backends
>     and their properties are defined in one place, but device models and
>     their properties aren't.  They get collected at run time.  As long
>     as the schema is fixed at compile-time, it can't express the
>     resulting tagged union.
> 
> Another good example: qom-get
> 
>     We have a return value '**'.
> 
>     The run time type is the type of the property identified by the
>     arguments.  Therefore, the compile time type can only be the union
>     of all property types, which effectively boils down to "anything".
> 
>     The only way to say "anything" right now is '**'.  Requires 'gen':
>     false.  I figure we could extend the generators to support '**' in a
>     few places, which may let us avoid 'gen': false here.
> 
> Drawback of '**': introspection knows nothing.
> 
> Introspection knowing nothing about netdev_add's and device_add's
> acceptable properties is a big, painful gap.
> 
> Please don't invent new reasons for 'gen': false without a very
> compelling use case.  If you think you have one, we need to talk to make
> sure use of 'gen': false really beats the alternatives.  Alternatives
> may include extending the generators.
> 
> 

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

[zhanghailiang]

On 2015/6/19 16:11, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> Hi,
>>
>> Is there any news about this discussion?
>> Is anyone working on it? ;)
>>
>> Since the 'hard feature freeze' time is closer, we'd better to fix it in 2.4
>> before libvirt uses it.
>>
>> I have sent a RFC patch "[RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension"
>> http://patchwork.ozlabs.org/patch/482125/
>> Which just changes qmp command to
>>   '{ "execute": "migrate-set-parameters" , "arguments":
>>       { "compression": { "compress-level": 1 } } }',
>> Compared with its original way, it seems to be more easy for reading and extension, but
>> it can't address most problems discussed here...
>
>>From a QMP wire protocol point of view, @migrate-set-parameters is just
> fine as it is:
>
>      #
>      # @migrate-set-parameters
>      #
>      # Set the following migration parameters
>      #
>      # @compress-level: compression level
>      #
>      # @compress-threads: compression thread count
>      #
>      # @decompress-threads: decompression thread count
>      #
>      # Since: 2.4
>      ##
>      { 'command': 'migrate-set-parameters',
>        'data': { '*compress-level': 'int',
>                  '*compress-threads': 'int',
>                  '*decompress-threads': 'int'} }
>
> However, as the number of parameters grows, the generated C interface
> becomes more and more unwieldy:
>
>      void qmp_migrate_set_parameters(bool has_compress_level,
>                                      int64_t compress_level,
>                                      bool has_compress_threads,
>                                      int64_t compress_threads,
>                                      bool has_decompress_threads,
>                                      int64_t decompress_threads,
>                                      ... more ...
>                                      Error **errp)
>

Yes, this is the problem.

> Your [RFC PATCH] hides away some of the parameters in a separate
> MigrationCompressParameter (misnamed, should be plural).  Leads to
>
>      void qmp_migrate_set_parameters(bool has_compress,
>                                      MigrationCompressParameters *compress,
>                                      ... more ...
>                                      Error **errp)
>
> Makes the QMP wire protocol more complex, because now you have to
> collect some parameters in extra curlies.
>
> Note the two levels of optionalness: one for the wrapping struct, one
> for each of its members.  If you made the members non-optional, we'd
> have to specify none or all, which is not what we want.  If you made the
> wrapping struct non-optional, you'd have to add a stupid 'compress': {}
> when you want to set something else.  So we need both.
>
> In C, the relatively simple test has_compress_level becomes compress &&
> compress->has_level.
>

Hmm, i see, thanks for your explanation.

> If we ever want to set migration parameters from the command line, the
> nesting will be in the way.  Our tool to parse command line into
> QAPI-generated data types (OptsVisitor) by design doesn't cope with
> nested structs.
>
> Perhaps the extra complexity is worthwhile, but it's certainly not
> obvious.
>
> If all we want is making adding new parameters easier (this thread's
> stated subject), then I guess having the function take a single struct
> parameter for all its arguments would do:
>
>      void qmp_migrate_set_parameters(MigrationParameters *parms,
>                                      Error **errp)
>
> To get that from the current QAPI code generator, we'd have to do
> something like
>
>      { 'command': 'migrate-set-parameters',
>        'data': { 'parms': 'MigrationParameters' } }
>
> Trouble is this messes up the QMP wire interface: you now have to wrap
> the actual arguments in two curlies instead of one.
>
> Not a backward-compatibility issue, because the command is new.
>
> Ugly all the same.
>
> To avoid the ugliness, we could change the QAPI generator.  Currently,
>
>      { 'command': 'migrate-set-parameters',
>        'data': 'MigrationParameters' }
>
> generates the same interface as when you inline MigrationParameters,
> namely
>
>      void qmp_migrate_set_parameters(bool has_compress_level,
>                                      int64_t compress_level,
>                                      bool has_compress_threads,
>                                      int64_t compress_threads,
>                                      bool has_decompress_threads,
>                                      int64_t decompress_threads,
>                                      ... more ...
>                                      Error **errp)
>
> It could instead generate
>
>      void qmp_migrate_set_parameters(MigrationParameters *parms,
>                                      Error **errp)
>
> No change to the wire protocol.  Fairly big, but relatively mechanical
> change to the handler functions.  I'd be willing to give it a shot and
> see how it turns out, but I can't do it for 2.4, sorry.
>

Great, that would be fine~ Thanks.

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

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> On 2015/6/19 16:11, Markus Armbruster wrote:
[...]
>> To avoid the ugliness, we could change the QAPI generator.  Currently,
>>
>>      { 'command': 'migrate-set-parameters',
>>        'data': 'MigrationParameters' }
>>
>> generates the same interface as when you inline MigrationParameters,
>> namely
>>
>>      void qmp_migrate_set_parameters(bool has_compress_level,
>>                                      int64_t compress_level,
>>                                      bool has_compress_threads,
>>                                      int64_t compress_threads,
>>                                      bool has_decompress_threads,
>>                                      int64_t decompress_threads,
>>                                      ... more ...
>>                                      Error **errp)
>>
>> It could instead generate
>>
>>      void qmp_migrate_set_parameters(MigrationParameters *parms,
>>                                      Error **errp)
>>
>> No change to the wire protocol.  Fairly big, but relatively mechanical
>> change to the handler functions.  I'd be willing to give it a shot and
>> see how it turns out, but I can't do it for 2.4, sorry.
>>
>
> Great, that would be fine~ Thanks.

I think I can give it a try myself some time in the 2.5 cycle.

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

[zhanghailiang]

On 2015/6/23 15:50, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> On 2015/6/19 16:11, Markus Armbruster wrote:
> [...]
>>> To avoid the ugliness, we could change the QAPI generator.  Currently,
>>>
>>>       { 'command': 'migrate-set-parameters',
>>>         'data': 'MigrationParameters' }
>>>
>>> generates the same interface as when you inline MigrationParameters,
>>> namely
>>>
>>>       void qmp_migrate_set_parameters(bool has_compress_level,
>>>                                       int64_t compress_level,
>>>                                       bool has_compress_threads,
>>>                                       int64_t compress_threads,
>>>                                       bool has_decompress_threads,
>>>                                       int64_t decompress_threads,
>>>                                       ... more ...
>>>                                       Error **errp)
>>>
>>> It could instead generate
>>>
>>>       void qmp_migrate_set_parameters(MigrationParameters *parms,
>>>                                       Error **errp)
>>>
>>> No change to the wire protocol.  Fairly big, but relatively mechanical
>>> change to the handler functions.  I'd be willing to give it a shot and
>>> see how it turns out, but I can't do it for 2.4, sorry.
>>>
>>
>> Great, that would be fine~ Thanks.
>
> I think I can give it a try myself some time in the 2.5 cycle.
>

OK, thanks~ :)

Re: [Qemu-devel] qapi: What does "'gen': false" actually do, and when should I use it?

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 09/06/2015 10:42, Markus Armbruster wrote:
>> Part 2: When to use it?
>> 
>> We use 'gen': false when we can't (be bothered to) specify the exact
>> type of an argument or result.
>> 
>> Bad example: netdev_add
>> 
>>     We have arguments 'type': 'str' and '*props': '**'.
>> 
>>     We should have a union tagged by network backend type.  For each
>>     type, the union holds the type's properties (if any).
>
> The problem with this is that netdev_add was not type safe, because it
> uses qemu_opts_from_qdict and QemuOpts is exclusively string-based.  So
> you could write 'port': '123' or 'port': 123, and both would work, the
> conversion to integer is done by the QemuOptsVisitor.
>
> Note that device_add would have the same problem.

Yes.

We can always start over with a new command and deprecate the old one.

Or we can add a backward-misfeature-compatible mode to QmpInputVisitor,
enabled only for the misfeatured commands.

Re: [Qemu-devel] qapi: What does "'gen': false" actually do, and when should I use it?

[Markus Armbruster]

Updating for future directions explored in "[PATCH RFC v2 00/47] qapi:
QMP introspection".

Markus Armbruster <armbru@redhat.com> writes:

> Two parts: 1. what does it do, and 2. when to use it.
>
>
> Part 1: What does it do?
>
> qapi-code-gen.txt explains it thus:
>
>     In rare cases, QAPI cannot express a type-safe representation of a
>     corresponding Client JSON Protocol command.  In these cases, if the
>     command expression includes the key 'gen' with boolean value false,
>     then the 'data' or 'returns' member that intends to bypass generated
>     type-safety and do its own manual validation should use an inline
>     dictionary definition, with a value of '**' rather than a valid type
>     name for the keys that the generated code will not validate.  Please
>     try to avoid adding new commands that rely on this, and instead use
>     type-safe unions.  For an example of bypass usage:
>
>      { 'command': 'netdev_add',
>        'data': {'type': 'str', 'id': 'str', '*props': '**'},
>        'gen': false }

With 'gen': false, 'data' and 'returns' aren't used by anything.  Good,
because this 'data' is actively misleading: we don't have an optional
'props' argument with "a list of properties" (quoting qapi-schema.json),
we have type-dependent property arguments that aren't specified here!

Example:

    { "execute": "netdev_add",
      "arguments": { "type": "user", "id": "netdev1",
                     "dnssearch": "example.org" } }

Incorrect example:

    { "execute": "netdev_add",
      "arguments": { "type": "user", "id": "netdev1",
                     "props": { "dnssearch": "example.org" } } }

"[PATCH RFC v2 42/47] qapi-schema: Fix up misleading specification of
netdev_add" deletes the misleading '*props': '**', and fixes up the
comment.

> I'm afraid that doesn't really answer the "what does it actually do?"
> question, so let's examine an example.  netdev_add isn't the best choice
> because it doesn't return anything, so let's use qom-get and qom-list.
>
> { 'command': 'qom-list',
>   'data': { 'path': 'str' },
>   'returns': [ 'ObjectPropertyInfo' ] }
>
> { 'command': 'qom-get',
>   'data': { 'path': 'str', 'property': 'str' },
>   'returns': '**',
>   'gen': false }
>
> qapi-commands.py generates qmp-marshal.c and qmp-commands.h.  For
> qom-list, it generates qmp_marshal_input_qom_list().  For qom-get, it
> generates nothing.

qom-get uses 'gen': false because it can return anything, and the schema
can't express that, so we have to cheat.  'returns': '**' is merely
documentation to remind us why we cheat.

"[PATCH RFC v2 40/47] qapi: Introduce a first class 'any' type" removes
the need to cheat, and "[PATCH RFC v2 41/47] qom: Don't use 'gen': false
for qom-get, qom-set, object-add" removes the cheating.  Result:

    { 'command': 'qom-get',
      'data': { 'path': 'str', 'property': 'str' },
      'returns': 'any' }

> We still rely on qmp-commands.hx to bind the monitor to the schema.  For
> qom-list, it binds to the generated function:
>
>     {
>         .name       = "qom-list",
>         .args_type  = "path:s",
>         .mhandler.cmd_new = qmp_marshal_input_qom_list,
>     },
>
> For qom-get, it binds to a hand-written one:
>
> {
>         .name       = "qom-get",
> 	.args_type  = "path:s,property:s",
> 	.mhandler.cmd_new = qmp_qom_get,
>     },
>
> Both are QMP command handlers, i.e their type is
>
>     void (*cmd_new)(QDict *params, QObject **ret_data, Error **errp);
>
> What does the generated handler do for us?  As the filename
> qmp-marshal.c suggests, it's basically a wrapper around a command
> handler with a "native C" interface that unmarshals from QDict to C
> "native C" data types and back.  For qom-list, the native handler is
>
>     ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
>     {
>         // Do stuff with the arguments, create the result
>         return props;
>     }
>
> This is the part you need to implement manually.
>
> For qom-get, you implement this one instead:
>
>     int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
>     {
>         // Unmarshall qdict into arguments
>         const char *path = qdict_get_str(qdict, "path");
>         const char *property = qdict_get_str(qdict, "property");
>         Error *local_err = NULL;
>         Object *obj;
>
>         // Do stuff with the arguments, create the result
>
>         // Marshal result into QObject
>         // nothing to do in this case, because it happens to be one
>         // already
>
>     out:
>         if (local_err) {
>             // Go from Error API to QMP's QError
>             // (QError will go away soon)
>             qerror_report_err(local_err);
>             error_free(local_err);
>             return -1;
>         }
>
>         return 0;
>     }
>
> Note the boilerplate around 'do stuff'.

This becomes:

    QObject *qmp_qom_get(const char *path, const char *property, Error **errp)
    {
        Object *obj;
    
        obj = object_resolve_path(path, NULL);
        if (!obj) {
            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                      "Device '%s' not found", path);
            return NULL;
        }
    
        return object_property_get_qobject(obj, property, errp);
    }

> Notably absent from the boilerplate: rejecting unknown parameters,
> checking mandatory parameters are present, checking types.  How come?
>
> For historical reasons, part of the unmarshalling job is done by QMP
> core, using args_type information from qmp-commands.hx.
>
> For qom-list, .args_type = "path:s".  The core checks argument "path" is
> present and is a JSON string, and no other argument is present.
>
> For qom-get, .args_type = "path:s,property:s".  I trust you can guess
> what the core checks there.
>
> When we eliminate qmp-commands.hx, the core's checking goes away, and
> needs to be replaced.
>
> Generated handlers like qmp_marshal_input_qom_list() already do full
> type checking of known parameters, so we just rely on that.  Handwritten
> ones like qmp_qom_get() generally don't.  They'll need to be updated to
> do it.

Either that, or lose their need for 'gen': false.

> Part 2: When to use it?
>
> We use 'gen': false when we can't (be bothered to) specify the exact
> type of an argument or result.
>
> Bad example: netdev_add
>
>     We have arguments 'type': 'str' and '*props': '**'.
>
>     We should have a union tagged by network backend type.  For each
>     type, the union holds the type's properties (if any).
>
> Better example: device_add (but that's not even QAPIfied, yet)
>
>     If QAPIfied, we'd have arguments like 'driver': 'str' and '*props':
>     '**'.

No, we won't have '*props'.  We'll have additional driver-dependent
arguments.

>     Looks just like netdev_add.  The difference is that network backends
>     and their properties are defined in one place, but device models and
>     their properties aren't.  They get collected at run time.  As long
>     as the schema is fixed at compile-time, it can't express the
>     resulting tagged union.
>
> Another good example: qom-get
>
>     We have a return value '**'.
>
>     The run time type is the type of the property identified by the
>     arguments.  Therefore, the compile time type can only be the union
>     of all property types, which effectively boils down to "anything".
>
>     The only way to say "anything" right now is '**'.  Requires 'gen':
>     false.  I figure we could extend the generators to support '**' in a
>     few places, which may let us avoid 'gen': false here.

s/ in a few places//

> Drawback of '**': introspection knows nothing.
>
> Introspection knowing nothing about netdev_add's and device_add's
> acceptable properties is a big, painful gap.
>
> Please don't invent new reasons for 'gen': false without a very
> compelling use case.  If you think you have one, we need to talk to make
> sure use of 'gen': false really beats the alternatives.  Alternatives
> may include extending the generators.

I added one in "[PATCH RFC v2 45/47] qapi: New QMP command query-schema
for QMP schema introspection":

    { 'command': 'query-schema',
      'returns': [ 'SchemaInfo' ],
      'gen': false }                # just to simplify qmp_query_json()

Partly because the simpler qmp_query_json() is more efficient, and
mostly because I'm lazy.  I'd gladly do without 'gen': false here if we
can then get rid of 'gen': false entirely.