Patchwork [07/18] qapi: Flat unions with arbitrary discriminator

login
register
mail settings
Submitter Kevin Wolf
Date July 23, 2013, 1:03 p.m.
Message ID <1374584606-5615-8-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/261083/
State New
Headers show

Comments

Kevin Wolf - July 23, 2013, 1:03 p.m.
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" }

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 docs/qapi-code-gen.txt | 22 +++++++++++++
 scripts/qapi-types.py  |  6 ++++
 scripts/qapi-visit.py  | 86 ++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 91 insertions(+), 23 deletions(-)
Eric Blake - July 26, 2013, 1:40 p.m.
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 <kwolf@redhat.com>
> ---
>  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 <eblake@redhat.com>
Kevin Wolf - July 26, 2013, 3:01 p.m.
Am 26.07.2013 um 15:40 hat Eric Blake geschrieben:
> 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 <kwolf@redhat.com>
> > ---
> >  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.

There's nothing to add to this, I would certainly support such a future
patch.

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

Sorry, this is in fact my own unfamiliarity with Python, combined with
failure to fix all cases when you pointed it out. The only reason I
didn't reply to that part of your review was that I thought it would be
obvious when I send a fixed version. Well, except if I don't.

I've changed this hunk now to match the other one:

@@ -184,8 +186,13 @@ struct %(name)s
 ''')
 
     if base:
-        struct = find_struct(base)
-        ret += generate_struct_fields(struct['data'])
+        base_fields = find_struct(base)['data']
+        if discriminator:
+            base_fields = base_fields.copy()
+            del base_fields[discriminator]
+        ret += generate_struct_fields(base_fields)
+    else:
+        assert not discriminator
 
     ret += mcgen('''
 };


> I think my findings are easy fixes; so I'm okay if you fix them and then
> add:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, Eric.

Kevin
Eric Blake - July 26, 2013, 3:13 p.m.
On 07/26/2013 09:01 AM, Kevin Wolf wrote:

>>>      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...
> 
> Sorry, this is in fact my own unfamiliarity with Python, combined with
> failure to fix all cases when you pointed it out. The only reason I
> didn't reply to that part of your review was that I thought it would be
> obvious when I send a fixed version. Well, except if I don't.

Ah, so this is a case of the blind leading the blind...

> 
> I've changed this hunk now to match the other one:
> 
> @@ -184,8 +186,13 @@ struct %(name)s
>  ''')
>  
>      if base:
> -        struct = find_struct(base)
> -        ret += generate_struct_fields(struct['data'])
> +        base_fields = find_struct(base)['data']
> +        if discriminator:
> +            base_fields = base_fields.copy()
> +            del base_fields[discriminator]
> +        ret += generate_struct_fields(base_fields)
> +    else:
> +        assert not discriminator

Yes, that makes more sense.

>> I think my findings are easy fixes; so I'm okay if you fix them and then
>> add:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>

Which means this is indeed a valid review.

Patch

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.
+The above example can then be modified as follows:
+
+ { 'type': 'BlockdevCommonOptions',
+   'data': { 'driver': 'str' 'readonly': 'bool' } }
+ { 'union': 'BlockdevOptions',
+   'base': 'BlockdevCommonOptions',
+   'discriminator': 'driver',
+   'data': { 'raw': 'RawOptions',
+             'qcow2': 'Qcow2Options' } }
+
+Resulting in this JSON object:
+
+ { "driver": "qcow2",
+   "readonly": false,
+   "backing-file": "/some/place/my-image",
+   "lazy-refcounts": true }
+
+
 === Commands ===
 
 Commands are defined by using a list containing three members.  The first
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9882b95..db2f533 100644
--- a/scripts/qapi-types.py
+++ 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]
         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
+++ b/scripts/qapi-visit.py
@@ -17,8 +17,26 @@  import os
 import getopt
 import errno
 
-def generate_visit_struct_fields(field_prefix, members):
+def generate_visit_struct_fields(name, field_prefix, members):
+    substructs = []
     ret = ''
+    full_name = name if not field_prefix else "%s_%s" % (name, field_prefix)
+
+    for argname, argentry, optional, structured in parse_args(members):
+        if structured:
+            ret += generate_visit_struct_fields(name, field_prefix + argname, argentry)
+
+    ret += mcgen('''
+
+static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp)
+{
+    Error *err = NULL;
+''',
+        name=name, full_name=full_name)
+    push_indent()
+
+    if len(field_prefix):
+        field_prefix = field_prefix + "."
 
     for argname, argentry, optional, structured in parse_args(members):
         if optional:
@@ -31,7 +49,7 @@  if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
             push_indent()
 
         if structured:
-            ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
+            ret += generate_visit_struct_body(full_name, argname, argentry)
         else:
             ret += mcgen('''
 visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
@@ -47,6 +65,12 @@  visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s",
 visit_end_optional(m, &err);
 ''')
 
+    pop_indent()
+    ret += mcgen('''
+
+    error_propagate(errp, err);
+}
+''')
     return ret
 
 
@@ -56,8 +80,9 @@  if (!error_is_set(errp)) {
 ''')
     push_indent()
 
+    full_name = name if not field_prefix else "%s_%s" % (field_prefix, name)
+
     if len(field_prefix):
-        field_prefix = field_prefix + "."
         ret += mcgen('''
 Error **errp = &err; /* from outer scope */
 Error *err = NULL;
@@ -74,20 +99,14 @@  visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
     ret += mcgen('''
 if (!err) {
     if (!obj || *obj) {
-''')
-    push_indent()
-    push_indent()
-
-    ret += generate_visit_struct_fields(field_prefix, members)
-    pop_indent()
-    ret += mcgen('''
-    error_propagate(errp, err);
-    err = NULL;
-}
-''')
+        visit_type_%(name)s_fields(m, obj, &err);
+        error_propagate(errp, err);
+        err = NULL;
+    }
+''',
+        name=full_name)
 
     pop_indent()
-    pop_indent()
     ret += mcgen('''
         /* Always call end_struct if start_struct succeeded.  */
         visit_end_struct(m, &err);
@@ -98,7 +117,9 @@  if (!err) {
     return ret
 
 def generate_visit_struct(name, members):
-    ret = mcgen('''
+    ret = generate_visit_struct_fields(name, "", members)
+
+    ret += mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
 {
@@ -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]
+        ret += generate_visit_struct_fields(name, "", base_fields)
+
     ret += mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
@@ -179,23 +208,34 @@  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
     push_indent()
 
     if base:
-        struct = find_struct(base)
-        push_indent()
-        ret += generate_visit_struct_fields("", struct['data'])
-        pop_indent()
+        ret += mcgen('''
+    visit_type_%(name)s_fields(m, obj, &err);
+''',
+            name=name)
 
     pop_indent()
     ret += mcgen('''
-        visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
+        visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
         if (!err) {
             switch ((*obj)->kind) {
 ''',
-                 name=name)
+                 name=name, type="type" if not discriminator else discriminator)
 
     for key in members:
+        if not discriminator:
+            fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);'
+        else:
+            fmt = '''visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(c_type)s), &err);
+                if (!err) {
+                    visit_type_%(c_type)s_fields(m, &(*obj)->%(c_name)s, &err);
+                    error_propagate(errp, err);
+                    err = NULL;
+                    visit_end_implicit_struct(m, &err);
+                }'''
+
         ret += mcgen('''
             case %(abbrev)s_KIND_%(enum)s:
-                visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+                ''' + fmt + '''
                 break;
 ''',
                 abbrev = de_camel_case(name).upper(),