Re: [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator

[Eric Blake <eblake@redhat.com>]

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

On 07/09/2013 03:53 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" }

[prior to reading the patch body]
I assume that the corresponding qapi-schema.json change that matches the
above wire context is:

{ 'type': 'Base',
  'data': { 'read-only': 'bool' } }
{ 'type': 'File',
  'data': { 'filename': 'str' } }
{ 'union': 'Example',
  'base': 'Base',
  'discriminator': 'driver',
  'data': { 'file': 'File' } }

with the new 'discriminator' element stating that an alternate name
(rather than the default 'type' name) is in use.  Do you allow flattened
unions always, or only when 'discriminator' was present?

Aren't you really flattening TWO structs?  That is, extending the above,
we are going from:

{ 'command': 'test', 'data': { 'driver': 'Example' } }

which is previously called as:

{ "execute": "test", "arguments": {
    "driver": { "type": "file", "read-only": true,
                "data": { "filename": "test" } } } }

to the shorter:

{ "execute": "test", "arguments": {
  "driver": "file", "read-only": true, "filename": "test" } }

[...now after reading the patch body]

Oh - you ONLY support a discriminator IF you pass a 'base' class, where
the discriminator has to be one of the fields named in the base class,
necessarily of type 'str'.  In other words, the qapi-schema.json needs
to look more like:

{ 'type': 'Base',
  'data': { 'read-only': 'bool', 'driver': 'str' } }
{ 'type': 'File',
  'data': { 'filename': 'str' } }
{ 'union': 'Example',
  'base': 'Base',
  'discriminator': 'driver',
  'data': { 'file': 'File' } }

If I'm off (or even if I'm right), more details in the commit message
about the qapi description that leads to the final QMP wire contents
would be helpful.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  scripts/qapi-types.py | 10 +++---
>  scripts/qapi-visit.py | 91 ++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 70 insertions(+), 31 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 960065b..db2f533 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -162,10 +162,8 @@ def generate_union(expr):
>      name = expr['union']
>      typeinfo = expr['data']
>  
> -    if expr.has_key('base'):
> -        base = expr['base']
> -    else:
> -        base = None
> +    base = expr.get('base')

Should this simplification be squashed into patch 2/11?  (Patch 4/11 has
a similar construct that might also be simplified.)

> +    discriminator = expr.get('discriminator')
>  
>      ret = mcgen('''
>  struct %(name)s
> @@ -189,7 +187,11 @@ struct %(name)s
>  
>      if base:
>          struct = find_struct(base)
> +        if discriminator:
> +            del struct['data'][discriminator]

Here my lack of python expertise shows, in what may be a dumb question:

Does find_struct(base) return a reference to the original object (oops -
modifying 'struct' here means that the next call to find_struct(base)
sees a smaller object), or a fully-cloned object (phew - our deletion is
to our local copy, but future callers get a fresh clone that still see
the discriminator)?

Meanwhile, should you assert that the discriminator is of type 'str'?
Actually, I suppose it might be nice to support a discriminator of an
enum type (to document the set of valid strings); but the point still
remains that the discriminator field is probably not going to work as a
bool, integer, or struct type.

-- 
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: 621 bytes --]