[Qemu-devel] [PATCH v8 15/16] qapi: Allow anonymous branch types in flat union

[Eric Blake]

Recent commits added support for an anonymous type as the base
of a flat union; with a bit more work, we can also allow an
anonymous struct as a branch of a flat union.  This probably
most useful when a branch adds no additional members beyond the
common elements of the base (that is, the branch struct is '{}'),
but can be used for any struct in the same way we allow for an
anonymous struct for a command.

The generator has to do a bit of special-casing for the fact that
we do not emit a 'q_empty' struct nor a 'visit_type_q_empty_members()'
corresponding to the special 'q_empty' type (see commit 7ce106a9 for
more reasons why); but when the branch is truly empty, there's nothing
to do.  This is easiest by declaring that an empty type member of
another QAPI type is emitted as a 'char' rather than bothering with a
named type, via c_unboxed_type() (this change also affects empty types
used as the branch of a simple union).

In QAPISchemaObjectTypeMember._pretty_owner(), we want to distinguish
which branch introduces any duplicate member with the base of a flat
union. We can't use just the branch name (otherwise, two flat unions
with the same branch name will create a collision in C types), so
we have to include the QAPI type name as well - but since both types
and branches can contain '-' (or even '_'), we need to introduce yet
another character for robust parsing of the generated name back to
the sources, so we use the character ':' in _make_variant() and
transliterate it to '_' in c_name_trans.

The testsuite gets an update to use the new feature, and to ensure
that we can still detect invalid collisions of QMP names.

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

---
v8: tweak commit message, drop unneeded hunks, change c_unboxed_type()
handling of empty types, don't lose test of flat union backref
v7: new patch
---
 scripts/qapi.py                          | 26 ++++++++++++++++++++------
 scripts/qapi-visit.py                    | 14 ++++++++++----
 tests/Makefile.include                   |  1 +
 tests/qapi-schema/flat-union-inline.err  |  2 +-
 tests/qapi-schema/flat-union-inline.json |  5 ++---
 tests/qapi-schema/qapi-schema-test.json  |  8 ++++++--
 tests/qapi-schema/qapi-schema-test.out   |  7 ++++++-
 tests/qapi-schema/union-inline.err       |  1 +
 tests/qapi-schema/union-inline.exit      |  1 +
 tests/qapi-schema/union-inline.json      |  4 ++++
 tests/qapi-schema/union-inline.out       |  0
 11 files changed, 52 insertions(+), 17 deletions(-)
 create mode 100644 tests/qapi-schema/union-inline.err
 create mode 100644 tests/qapi-schema/union-inline.exit
 create mode 100644 tests/qapi-schema/union-inline.json
 create mode 100644 tests/qapi-schema/union-inline.out

diff --git a/scripts/qapi.py b/scripts/qapi.py
index e051892..db115eb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -607,9 +607,11 @@ def check_union(expr, expr_info):
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)

-        # Each value must name a known type
+        # Each value must name a type; although the type may be anonymous
+        # for a flat union.
         check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
-                   value, allow_array=not base, allow_metas=allow_metas)
+                   value, allow_array=not base, allow_dict=base is not None,
+                   allow_optional=True, allow_metas=allow_metas)

         # If the discriminator names an enum type, then all members
         # of 'data' must also be members of the enum type.
@@ -1022,6 +1024,8 @@ class QAPISchemaObjectType(QAPISchemaType):
         return c_name(self.name) + pointer_suffix

     def c_unboxed_type(self):
+        if self.is_empty():
+            return 'char'
         return c_name(self.name)

     def json_type(self):
@@ -1067,6 +1071,11 @@ class QAPISchemaMember(object):
                 return '(parameter of %s)' % owner[:-4]
             elif owner.endswith('-base'):
                 return '(base of %s)' % owner[:-5]
+            elif owner.endswith('-branch'):
+                # See QAPISchema._make_variant() for the further division
+                # of the owner
+                return ('(member of %s branch %s)'
+                        % tuple(owner[:-7].split(':')))
             else:
                 assert owner.endswith('-wrapper')
                 # Unreachable and not implemented
@@ -1362,7 +1371,12 @@ class QAPISchema(object):
                                               self._make_members(data, info),
                                               None))

-    def _make_variant(self, case, typ):
+    def _make_variant(self, case, typ, info, owner):
+        if isinstance(typ, dict):
+            # See also QAPISchemaObjectTypeMember._pretty_owner()
+            typ = self._make_implicit_object_type(
+                "%s:%s" % (owner, case), info, 'branch',
+                self._make_members(typ, info)) or 'q_empty'
         return QAPISchemaObjectTypeVariant(case, typ)

     def _make_simple_variant(self, case, typ, info):
@@ -1383,7 +1397,7 @@ class QAPISchema(object):
             base = (self._make_implicit_object_type(
                     name, info, 'base', self._make_members(base, info)))
         if tag_name:
-            variants = [self._make_variant(key, value)
+            variants = [self._make_variant(key, value, info, name)
                         for (key, value) in data.iteritems()]
             members = []
         else:
@@ -1402,7 +1416,7 @@ class QAPISchema(object):
     def _def_alternate_type(self, expr, info):
         name = expr['alternate']
         data = expr['data']
-        variants = [self._make_variant(key, value)
+        variants = [self._make_variant(key, value, info, name)
                     for (key, value) in data.iteritems()]
         tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
         self._def_entity(
@@ -1512,7 +1526,7 @@ def c_enum_const(type_name, const_name, prefix=None):
         type_name = prefix
     return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()

-c_name_trans = string.maketrans('.-', '__')
+c_name_trans = string.maketrans('.-:', '___')


 # Map @name to a valid C identifier.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 96f2491..9fa1f04 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -83,13 +83,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
*obj, Error **errp)
         for var in variants.variants:
             ret += mcgen('''
     case %(case)s:
-        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
-        break;
 ''',
                          case=c_enum_const(variants.tag_member.type.name,
                                            var.name,
-                                           variants.tag_member.type.prefix),
-                         c_type=var.type.c_name(), c_name=c_name(var.name))
+                                           variants.tag_member.type.prefix))
+            if (not isinstance(var.type, QAPISchemaObjectType) or
+                    not var.type.is_empty()):
+                ret += mcgen('''
+        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
+''',
+                             c_type=var.type.c_name(), c_name=c_name(var.name))
+            ret += mcgen('''
+        break;
+''')

         ret += mcgen('''
     default:
diff --git a/tests/Makefile.include b/tests/Makefile.include
index f73f350..8a4c60d 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -394,6 +394,7 @@ qapi-schema += union-base-no-discriminator.json
 qapi-schema += union-branch-case.json
 qapi-schema += union-clash-branches.json
 qapi-schema += union-empty.json
+qapi-schema += union-inline.json
 qapi-schema += union-invalid-base.json
 qapi-schema += union-optional-branch.json
 qapi-schema += union-unknown.json
diff --git a/tests/qapi-schema/flat-union-inline.err 
b/tests/qapi-schema/flat-union-inline.err
index 2333358..efcafec 100644
--- a/tests/qapi-schema/flat-union-inline.err
+++ b/tests/qapi-schema/flat-union-inline.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 
'TestUnion' should be a type name
+tests/qapi-schema/flat-union-inline.json:6: 'kind' (member of TestUnion branch 
value2) collides with 'kind' (member of Base)
diff --git a/tests/qapi-schema/flat-union-inline.json 
b/tests/qapi-schema/flat-union-inline.json
index 62c7cda..a049ec8 100644
--- a/tests/qapi-schema/flat-union-inline.json
+++ b/tests/qapi-schema/flat-union-inline.json
@@ -1,5 +1,4 @@
-# we require branches to be a struct name
-# TODO: should we allow anonymous inline branch types?
+# we allow anonymous union branches, but they must not have clashing names
 { 'enum': 'TestEnum',
   'data': [ 'value1', 'value2' ] }
 { 'struct': 'Base',
@@ -8,4 +7,4 @@
   'base': 'Base',
   'discriminator': 'enum1',
   'data': { 'value1': { 'string': 'str' },
-            'value2': { 'integer': 'int' } } }
+            'value2': { 'kind': 'int' } } }
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 919dc097..c527af0 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -23,7 +23,7 @@
 # for testing override of default naming heuristic
 { 'enum': 'QEnumTwo',
   'prefix': 'QENUM_TWO',
-  'data': [ 'value1', 'value2' ] }
+  'data': [ 'value1', 'value2', 'value3', 'value4' ] }

 # for testing nested structs
 { 'struct': 'UserDefOne',
@@ -81,7 +81,11 @@
   'base': { '*integer': 'int', 'string': 'str', 'enum1': 'QEnumTwo' },
   'discriminator': 'enum1',
   'data': { 'value1' : 'UserDefC', # intentional forward reference
-            'value2' : 'UserDefB' } }
+            'value2' : 'UserDefB', # intentional back reference
+            'value3' : { },
+            'value4' : { 'intb': 'int', '*a-b': 'bool' }
+            # value2 and value4 have same members, but do not collide
+  } }

 { 'struct': 'WrapAlternate',
   'data': { 'alt': 'UserDefAlternate' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index e7ea242..fb4ca88 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -52,7 +52,7 @@ object NestedEnumsOne
     member enum2: EnumOne optional=True
     member enum3: EnumOne optional=False
     member enum4: EnumOne optional=True
-enum QEnumTwo ['value1', 'value2']
+enum QEnumTwo ['value1', 'value2', 'value3', 'value4']
     prefix QENUM_TWO
 enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 
'qbool']
     prefix QTYPE
@@ -85,6 +85,8 @@ object UserDefFlatUnion2
     tag enum1
     case value1: UserDefC
     case value2: UserDefB
+    case value3: q_empty
+    case value4: q_obj_UserDefFlatUnion2:value4-branch
 object UserDefNativeListUnion
     member type: UserDefNativeListUnionKind optional=False
     tag type
@@ -179,6 +181,9 @@ object q_obj_UserDefFlatUnion2-base
     member integer: int optional=True
     member string: str optional=False
     member enum1: QEnumTwo optional=False
+object q_obj_UserDefFlatUnion2:value4-branch
+    member intb: int optional=False
+    member a-b: bool optional=True
 object q_obj___org.qemu_x-command-arg
     member a: __org.qemu_x-EnumList optional=False
     member b: __org.qemu_x-StructList optional=False
diff --git a/tests/qapi-schema/union-inline.err 
b/tests/qapi-schema/union-inline.err
new file mode 100644
index 0000000..6c5389a
--- /dev/null
+++ b/tests/qapi-schema/union-inline.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-inline.json:2: Member 'value1' of union 'TestUnion' 
should be a type name
diff --git a/tests/qapi-schema/union-inline.exit 
b/tests/qapi-schema/union-inline.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-inline.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-inline.json 
b/tests/qapi-schema/union-inline.json
new file mode 100644
index 0000000..b8c5df6
--- /dev/null
+++ b/tests/qapi-schema/union-inline.json
@@ -0,0 +1,4 @@
+# simple unions cannot have anonymous branches (only flat unions can)
+{ 'union': 'TestUnion',
+  'data': { 'value1': { 'string': 'str' },
+            'value2': { 'kind': 'int' } } }
diff --git a/tests/qapi-schema/union-inline.out 
b/tests/qapi-schema/union-inline.out
new file mode 100644
index 0000000..e69de29
-- 
2.5.5