Re: [Qemu-devel] [PATCH 07/18] qapi: Flat unions with arbitrary discriminator

[Eric Blake]

On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> Instead of the rather verbose syntax that distinguishes base and
> subclass fields...
> 
>   { "type": "file",
>     "read-only": true,
>     "data": {
>         "filename": "test"
>     } }
> 
> ...we can now have both in the same namespace, allowing a more direct
> mapping of the command line, and moving fields between the common base
> and subclasses without breaking the API:
> 
>   { "driver": "file",
>     "read-only": true,
>     "filename": "test" }

MUCH nicer!

> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  docs/qapi-code-gen.txt | 22 +++++++++++++
>  scripts/qapi-types.py  |  6 ++++
>  scripts/qapi-visit.py  | 86 
> ++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 91 insertions(+), 23 deletions(-)

> 
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 555ca66..c187fda 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -103,6 +103,28 @@ And it looks like this on the wire:
>     "data" : { "backing-file": "/some/place/my-image",
>                "lazy-refcounts": true } }
>  
> +
> +Flat union types avoid the nesting on the wire. They are used whenever a
> +specific field of the base type is declared as the discriminator ('type' is
> +then no longer generated). The discriminator must always be a string field.

Since an 'enum' is always sent over the wire as a string, is it
appropriate to allow the discriminator to be an enum field instead of a
'str'?  But that could be done in a followup patch; your initial usage
is just fine with 'str'.  Besides, if we allow the discriminator to have
'enum' type, that would imply that we want to guarantee coverage that
all of the 'data' members of the union type correspond to the members of
the union.  On the other hand, that would be extra type safety - if we
extend the enum that backs the discriminator, we'd immediately be
reminded if we forgot to also extend the union based on that enum.
Again, food for thought for a future patch, and not something needed for
this one.

> +The above example can then be modified as follows:
> +
> + { 'type': 'BlockdevCommonOptions',
> +   'data': { 'driver': 'str' 'readonly': 'bool' } }

Missing a comma.

> +++ b/scripts/qapi-types.py
> @@ -161,7 +161,9 @@ def generate_union(expr):
>  
>      name = expr['union']
>      typeinfo = expr['data']
> +
>      base = expr.get('base')
> +    discriminator = expr.get('discriminator')
>  
>      ret = mcgen('''
>  struct %(name)s
> @@ -185,7 +187,11 @@ struct %(name)s
>  
>      if base:
>          struct = find_struct(base)
> +        if discriminator:
> +            del struct['data'][discriminator]

I asked before, but didn't get an answer; my question may just show my
unfamiliarity with python.  Is this modifying the original 'struct',
such that other uses of the struct after this point will no longer
contain the discriminator key?  Or is it only modifying a copy of
'struct', with the original left intact?  But based on the rest of your
patch...

>          ret += generate_struct_fields(struct['data'])
> +    else:
> +        assert not discriminator
>  
>      ret += mcgen('''
>  };
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index db6fa44..cd33e44 100644
> --- a/scripts/qapi-visit.py

> @@ -157,9 +178,17 @@ def generate_visit_union(expr):
>      members = expr['data']
>  
>      base = expr.get('base')
> +    discriminator = expr.get('discriminator')
>  
>      ret = generate_visit_enum('%sKind' % name, members.keys())
>  
> +    if base:
> +        base_fields = find_struct(base)['data']
> +        if discriminator:
> +            base_fields = base_fields.copy()
> +            del base_fields[discriminator]

...here, you explicitly took a copy to be safe.  So I suspect you are
missing a .copy() above.

I think my findings are easy fixes; so I'm okay if you fix them and then
add:

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