[v2,03/49] qapi: convert NumaOptions into a flat union

Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
---
 numa.c           |  2 +-
 qapi-schema.json | 47 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 12 deletions(-)

On Fri, Aug 21, 2015 at 05:36:59PM +0200, Kővágó, Zoltán wrote:
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>

I don't understand QAPI enough to understand why exactly this is needed
(so I would like to get feedback from somebody who actually understands
QAPI unions), but I have one comment below.

[...]
>  ##
> +# @NumaDriver
> +#
> +# List of possible numa drivers.
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'NumaDriver',
> +  'data': [ 'node' ] }

Why is the name "NumaDriver"? Below, the field is called "type", so why
not something like "NumaOptionType"?

> +
> +##
> +# @NumaCommonOptions
> +#
> +# Common set of numa options.
> +#
> +# @type: the numa driver to use
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'NumaCommonOptions',
> +  'data': {
> +    'type': 'NumaDriver' } }
> +
> +##
> +# @NumaOptions
> +#
> +# A discriminated record of NUMA options. (for OptsVisitor)
> +#
> +# Since 2.1
> +##
> +{ 'union': 'NumaOptions',
> +  'base': 'NumaCommonOptions',
> +  'discriminator': 'type',
> +  'data': {
> +    'node': 'NumaNodeOptions' }}
> +
> +##
>  # @HostMemPolicy
>  #
>  # Host memory policy types
> -- 
> 2.5.0
>

2015-08-22 01:13 keltezéssel, Eduardo Habkost írta:
> On Fri, Aug 21, 2015 at 05:36:59PM +0200, Kővágó, Zoltán wrote:
>> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
>
> I don't understand QAPI enough to understand why exactly this is needed
> (so I would like to get feedback from somebody who actually understands
> QAPI unions), but I have one comment below.

It's needed so the option visitor can support nested structures properly 
without flattening them (or breaking backward compatibility).

There is some discussion here:
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04438.html

But basically the thing is that with new new opts visitor, unless we 
convert NumaOptions into a flat union, the user would have to type -numa 
node,node.nodeid=foo instead of -numa node,nodeid=foo which would break 
backward compatibility.

>
> [...]
>>   ##
>> +# @NumaDriver
>> +#
>> +# List of possible numa drivers.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'enum': 'NumaDriver',
>> +  'data': [ 'node' ] }
>
> Why is the name "NumaDriver"? Below, the field is called "type", so why
> not something like "NumaOptionType"?

No particular reason other than the example in docs/qapi-code-gen.txt 
used driver.  The field is called type because in the non-flat union the 
discriminator is called type.

>> +
>> +##
>> +# @NumaCommonOptions
>> +#
>> +# Common set of numa options.
>> +#
>> +# @type: the numa driver to use
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'struct': 'NumaCommonOptions',
>> +  'data': {
>> +    'type': 'NumaDriver' } }
>> +
>> +##
>> +# @NumaOptions
>> +#
>> +# A discriminated record of NUMA options. (for OptsVisitor)
>> +#
>> +# Since 2.1
>> +##
>> +{ 'union': 'NumaOptions',
>> +  'base': 'NumaCommonOptions',
>> +  'discriminator': 'type',
>> +  'data': {
>> +    'node': 'NumaNodeOptions' }}
>> +
>> +##
>>   # @HostMemPolicy
>>   #
>>   # Host memory policy types
>> --
>> 2.5.0
>>
>

On Sat, Aug 22, 2015 at 05:56:40PM +0200, Kővágó Zoltán wrote:
> 2015-08-22 01:13 keltezéssel, Eduardo Habkost írta:
> >On Fri, Aug 21, 2015 at 05:36:59PM +0200, Kővágó, Zoltán wrote:
> >>Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> >
> >I don't understand QAPI enough to understand why exactly this is needed
> >(so I would like to get feedback from somebody who actually understands
> >QAPI unions), but I have one comment below.
> 
> It's needed so the option visitor can support nested structures properly
> without flattening them (or breaking backward compatibility).
> 
> There is some discussion here:
> http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04438.html
> 
> But basically the thing is that with new new opts visitor, unless we convert
> NumaOptions into a flat union, the user would have to type -numa
> node,node.nodeid=foo instead of -numa node,nodeid=foo which would break
> backward compatibility.

As long as somebody who understands QAPI says the changes make sense and
should work, I am OK with them if the following is changed:

> >[...]
> >>  ##
> >>+# @NumaDriver
> >>+#
> >>+# List of possible numa drivers.
> >>+#
> >>+# Since: 2.5
> >>+##
> >>+{ 'enum': 'NumaDriver',
> >>+  'data': [ 'node' ] }
> >
> >Why is the name "NumaDriver"? Below, the field is called "type", so why
> >not something like "NumaOptionType"?
> 
> No particular reason other than the example in docs/qapi-code-gen.txt used
> driver.  The field is called type because in the non-flat union the
> discriminator is called type.

In the docs/qapi-code-gen.txt example, the option is about an actual
blockdev driver, and the discriminator is really called "driver".

In this case, the field is called "type" and has nothing to do with
drivers, so something like NumaOptionType makes more sense.

On 08/21/2015 09:36 AM, Kővágó, Zoltán wrote:
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>

The subject line is a one-line "what", and the commit body should be the
"why" - all but the most trivial "what" deserve a non-empty commit body.

In particular, I think you NEED to mention in the commit body that the
conversion does not strictly follow the QMP wire equivalency between
simple and flat unions as spelled out in docs/qapi-code-gen.txt, but
that this is okay because we do not have any QMP commands that use the
NumaOptions type to begin with (so there is no observable change to any
commands sent over QMP).  Meanwhile, mention that the change is made to
convert the generated C code into something that will ultimately be
easier to work with.

> ---
>  numa.c           |  2 +-
>  qapi-schema.json | 47 ++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 37 insertions(+), 12 deletions(-)
> 

> 
> +++ b/numa.c
> @@ -227,7 +227,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
>      }
>  
>      switch (object->kind) {
> -    case NUMA_OPTIONS_KIND_NODE:
> +    case NUMA_DRIVER_NODE:

Now that commit 0f61af3 has landed, you'll have to rebase your patch to
use 'switch (object->type) {' here.  Basically, the conversion from
simple to flat union now (temporarily) generates a different tag name
for simple unions than for flat unions (although I have a pending patch
[1] that solves that, but by using object->type everywhere).

[1] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02061.html


> +++ b/qapi-schema.json
> @@ -3536,17 +3536,6 @@
>    'data': { '*console':'int', 'events': [ 'InputEvent' ] } }
>  
>  ##
> -# @NumaOptions
> -#
> -# A discriminated record of NUMA options. (for OptsVisitor)
> -#
> -# Since 2.1
> -##
> -{ 'union': 'NumaOptions',
> -  'data': {
> -    'node': 'NumaNodeOptions' }}

If we were following the equivalency documented in
docs/qapi-code-gen.txt, then this part is okay (you are removing the
simple union),...

>  ##
> +# @NumaDriver
> +#
> +# List of possible numa drivers.
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'NumaDriver',
> +  'data': [ 'node' ] }

...this part is okay (you are creating a new enum, which covers all
branches present in the former simple union),...

> +
> +##
> +# @NumaCommonOptions
> +#
> +# Common set of numa options.
> +#
> +# @type: the numa driver to use
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'NumaCommonOptions',
> +  'data': {
> +    'type': 'NumaDriver' } }

...this part is okay (you are creating a new base type, which consists
of a single common member named 'type' of the new enum type),...

[By the way, while the base type is currently required, I have a pending
patch that would add syntax sugar to avoid it, so maybe you want to wait
for my patch [2] to land?

[2] https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02321.html

so you could write 'base': { 'type': 'NumaDriver' } directly in the flat
union, instead of having to declare NumaCommonOptions]

> +
> +##
> +# @NumaOptions
> +#
> +# A discriminated record of NUMA options. (for OptsVisitor)
> +#
> +# Since 2.1
> +##
> +{ 'union': 'NumaOptions',
> +  'base': 'NumaCommonOptions',
> +  'discriminator': 'type',
> +  'data': {
> +    'node': 'NumaNodeOptions' }}

...but this part is missing one piece of the conversion (to be strictly
QMP compatible with the simple union, each branch of the flat union must
be a NEW type that contains a single member 'data' of the old type).

That is, if we were TRYING to preserve QMP compatibility, we'd need:

{ 'struct': 'NumaNodeOptionsWrapper',
  'data': { 'data': 'NumaNodeOptions' } }
{ 'union': 'NumaOptions', 'base': 'NumaCommonOptions',
  'discriminator': 'type', 'data': {
    'node': 'NumaNodeOptionsWrapper' } }

The difference on the wire is that the old simple union, or with my
version of the flat union, would allow:

{ 'command': 'foo', 'arguments': {
    'type': 'numa', 'data': { 'nodeid': 1 } } }

while the new flat union as you wrote it reduces nesting:

{ 'command': 'foo', 'arguments': {
    'type': 'numa', 'nodeid': 1 } }


That said, there is another point of view to worry about, which is what
C structures get generated.  Here, I'm using output after Markus' v4
series is applied (since it is slightly more legible than current
qemu.git mainline generated output) (and although I still have further
pending patches that improve it further).  The pre-patch simple union
would generate:

struct NumaOptions {
    NumaOptionsKind kind;
    union { /* union tag is @kind */
        void *data;
        NumaNodeOptions *node;
    };
};

and your version would generate (notice the rename from 'kind' to 'type'):

struct NumaOptions {
    /* Members inherited from NumaCommonOptions: */
    NumaDriver type;
    /* Own members: */
    union { /* union tag is @type */
        void *data;
        NumaNodeOptions *node;
    };
};

but my proposal to keep QMP compatibility would generate an incompatible
layer of boxing:

struct NumaOptions {
    /* Members inherited from NumaCommonOptions: */
    NumaDriver type;
    /* Own members: */
    union { /* union tag is @type */
        void *data;
        NumaNodeOptionsWrapper *node;
    };
};


So, in conclusion, since no existing QMP command used the old type, and
your choice of changes preserved the generated struct layout, it means
we have no change to command line parsing and no user-visible changes to
worry about.  And I'm fine with this patch, once you improve the commit
message and rebase to mainline:

Reviewed-by: Eric Blake <eblake@redhat.com>

On 08/26/2015 09:31 AM, Eduardo Habkost wrote:
> As long as somebody who understands QAPI says the changes make sense and
> should work, I am OK with them if the following is changed:

That would be me, right? See my other mail for my tentative R-b;
although if you start renaming things due to this discussion, maybe I
should review v3 more closely after all.

>>>> +##
>>>> +{ 'enum': 'NumaDriver',
>>>> +  'data': [ 'node' ] }
>>>
>>> Why is the name "NumaDriver"? Below, the field is called "type", so why
>>> not something like "NumaOptionType"?
>>
>> No particular reason other than the example in docs/qapi-code-gen.txt used
>> driver.  The field is called type because in the non-flat union the
>> discriminator is called type.
> 
> In the docs/qapi-code-gen.txt example, the option is about an actual
> blockdev driver, and the discriminator is really called "driver".
> 
> In this case, the field is called "type" and has nothing to do with
> drivers, so something like NumaOptionType makes more sense.

The enum name is not ABI (we can name it whatever makes sense), but does
show up in the C code that interacts with the generated type.  In fact,
if you could name the enum type 'NumaOptionsKind', then the C enum name
would not change (staying at NUMA_OPTIONS_KIND_NODE instead of your
change to NUMA_DRIVER_NODE).  Except that right now, the qapi generator
doesn't allow user-defined types ending in 'Kind'.  :(

I don't know if naming it NumaDriver makes sense; and so your suggestion
of naming it NumaOptionsType may be better in the long run.  It would
make your C enum become NUMA_OPTIONS_TYPE_NODE.

Or maybe you wait for me to do a followup patch to the qapi generator to
allow user-defined types ending in 'Kind'.