Re: [Qemu-devel] [PATCH v2 03/49] qapi: convert NumaOptions into a flat union

[Eric Blake]

On 08/21/2015 09:36 AM, Kővágó, Zoltán wrote:
> Signed-off-by: Kővágó, Zoltán <address@hidden>

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 <address@hidden>

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

signature.asc
Description: OpenPGP digital signature