[Qemu-devel] [PATCH v9 16/17] qapi: Finish converting to new qapi union layout

[Eric Blake]

We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.

This patch is the back end for a series that converts to a
saner qapi union layout.  Now that all clients have been
converted to use 'type' and 'obj->u.value', we can drop the
temporary support for 'kind' and 'obj->value', and undo the
temporary restriction against 'u' as a branch name.

Given a simple union qapi type:

{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }

this is the overall effect, when compared to the state before
this series of patches:

| struct Foo {
|-    FooKind kind;
|-    union { /* union tag is @kind */
|+    FooKind type;
|+    union { /* union tag is @type */
|         void *data;
|         int64_t a;
|         bool b;
|-    };
|+    } u;
| };

Note, however, that we do not rename the generated enum, which
is still 'FooKind'.  A further patch could generate implicit
enums as 'FooType', but that causes more churn to C code, and
gets harder since the generator already reserved the '*Kind'
namespace, but there are already QMP constructs with '*Type'
naming which means we cannot easily reserve it for qapi.

Signed-off-by: Eric Blake <address@hidden>

---
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
 scripts/qapi-types.py                   | 26 +++++---------------------
 tests/qapi-schema/qapi-schema-test.json |  4 +---
 tests/qapi-schema/qapi-schema-test.out  |  4 ++--
 3 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 0a14451..4ba39b6 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -136,23 +136,10 @@ struct %(c_name)s {
     if base:
         ret += gen_struct_fields([], base)
     else:
-        # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
-        # want to use only 'type', but the conversion is large enough to
-        # require staging over several commits.
-        ret += mcgen('''
-    union {
-        %(c_type)s kind;
-        %(c_type)s type;
-    };
-''',
-                     c_type=c_name(variants.tag_member.type.name))
+        ret += gen_struct_field(variants.tag_member.name,
+                                variants.tag_member.type,
+                                False)

-    # TODO As a hack, we emit the union twice, once as an anonymous union
-    # and once as a named union.  Ultimately, we want to use only the
-    # named union version (as it avoids conflicts between tag values as
-    # branch names competing with non-variant QMP names), but the conversion
-    # is large enough to require staging over several commits.
-    tmp = ''
     # FIXME: What purpose does data serve, besides preventing a union that
     # has a branch named 'data'? We use it in qapi-visit.py to decide
     # whether to bypass the switch statement if visiting the discriminator
@@ -161,7 +148,7 @@ struct %(c_name)s {
     # should not be any data leaks even without a data pointer.  Or, if
     # 'data' is merely added to guarantee we don't have an empty union,
     # shouldn't we enforce that at .json parse time?
-    tmp += mcgen('''
+    ret += mcgen('''
     union { /* union tag is @%(c_name)s */
         void *data;
 ''',
@@ -170,17 +157,14 @@ struct %(c_name)s {
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
         typ = var.simple_union_type() or var.type
-        tmp += mcgen('''
+        ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
                      c_type=typ.c_type(),
                      c_name=c_name(var.name))

-    ret += tmp
-    ret += '    ' + '\n    '.join(tmp.split('\n'))
     ret += mcgen('''
     } u;
-    };
 };
 ''')

diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 876ce18..22e15eb 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -113,10 +113,8 @@
 # should still be valid as a type or union branch name. And although
 # '*Kind' and '*List' are forbidden as type names, they should not be
 # forbidden as a member or branch name.
-# TODO - we temporarily do not support 'u' as branch name, while converting
-# code to use the new union layout
 { 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
-{ 'union': 'u', 'data': { 'u8': 'uint8', 'myKind': 'has_a',
+{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
                           'myList': 'has_a' } }

 # testing commands
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index cb12435..feaf20d 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -202,10 +202,10 @@ object has_a
     member MyKind: int optional=False
     member MyList: intList optional=False
 object u
-    case u8: :obj-uint8-wrapper
+    case u: :obj-uint8-wrapper
     case myKind: :obj-has_a-wrapper
     case myList: :obj-has_a-wrapper
-enum uKind ['u8', 'myKind', 'myList']
+enum uKind ['u', 'myKind', 'myList']
 command user_def_cmd None -> None
    gen=True success_response=True
 command user_def_cmd1 :obj-user_def_cmd1-arg -> None
-- 
2.4.3