qapi: Tighten checking of unions

Previous commits demonstrated that the generator had several
flaws with less-than-perfect unions:
- a simple union that listed the same branch twice (or two variant
names that map to the same C enumerator, including the implicit
MAX sentinel) ended up generating invalid C code
- an anonymous union that listed two branches with the same qtype
ended up generating invalid C code
- the generator crashed on anonymous union attempts to use an
array type
- the generator was silently ignoring a base type for anonymous
unions
- the generator allowed unknown types or nested anonymous unions
as a branch in an anonymous union

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>

2 files changed, 74 insertions, 28 deletions

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f6fb930279..2390887f28 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -181,17 +181,8 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
     name=name)
 
     for key in members:
-        qapi_type = members[key]
-        if builtin_types.has_key(qapi_type):
-            qtype = builtin_types[qapi_type]
-        elif find_struct(qapi_type):
-            qtype = "QTYPE_QDICT"
-        elif find_union(qapi_type):
-            qtype = "QTYPE_QDICT"
-        elif find_enum(qapi_type):
-            qtype = "QTYPE_QSTRING"
-        else:
-            assert False, "Invalid anonymous union member"
+        qtype = find_anonymous_member_qtype(members[key])
+        assert qtype, "Invalid anonymous union member"
 
         ret += mcgen('''
     [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 438468e3aa..5f0f699994 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -224,6 +224,23 @@ def find_base_fields(base):
         return None
     return base_struct_define['data']
 
+# Return the qtype of an anonymous union branch, or None on error.
+def find_anonymous_member_qtype(qapi_type):
+    if builtin_types.has_key(qapi_type):
+        return builtin_types[qapi_type]
+    elif find_struct(qapi_type):
+        return "QTYPE_QDICT"
+    elif find_enum(qapi_type):
+        return "QTYPE_QSTRING"
+    else:
+        union = find_union(qapi_type)
+        if union:
+            discriminator = union.get('discriminator')
+            if discriminator == {}:
+                return None
+            return "QTYPE_QDICT"
+    return None
+
 # Return the discriminator enum define if discriminator is specified as an
 # enum type, otherwise return None.
 def discriminator_find_enum_define(expr):
@@ -258,6 +275,8 @@ def check_union(expr, expr_info):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
+    values = { 'MAX': '(automatic)' }
+    types_seen = {}
 
     # If the object has a member 'base', its value must name a complex type,
     # and there must be a discriminator.
@@ -266,26 +285,35 @@ def check_union(expr, expr_info):
             raise QAPIExprError(expr_info,
                                 "Union '%s' requires a discriminator to go "
                                 "along with base" %name)
-        base_fields = find_base_fields(base)
-        if not base_fields:
-            raise QAPIExprError(expr_info,
-                                "Base '%s' is not a valid type"
-                                % base)
 
     # If the union object has no member 'discriminator', it's a
     # simple union. If 'discriminator' is {}, it is an anonymous union.
-    if not discriminator or discriminator == {}:
+    if discriminator is None or discriminator == {}:
         enum_define = None
+        if base is not None:
+            raise QAPIExprError(expr_info,
+                                "Union '%s' must not have a base"
+                                % name)
 
     # Else, it's a flat union.
     else:
-        # The object must have a member 'base'.
-        if not base:
+        # The object must have a string member 'base'.
+        if not isinstance(base, str):
             raise QAPIExprError(expr_info,
-                                "Flat union '%s' must have a base field"
+                                "Flat union '%s' must have a string base field"
                                 % name)
+        base_fields = find_base_fields(base)
+        if not base_fields:
+            raise QAPIExprError(expr_info,
+                                "Base '%s' is not a valid type"
+                                % base)
+
         # The value of member 'discriminator' must name a member of the
         # base type.
+        if not isinstance(discriminator, str):
+            raise QAPIExprError(expr_info,
+                                "Flat union '%s' discriminator must be a string"
+                                % name)
         discriminator_type = base_fields.get(discriminator)
         if not discriminator_type:
             raise QAPIExprError(expr_info,
@@ -301,15 +329,42 @@ def check_union(expr, expr_info):
 
     # Check every branch
     for (key, value) in members.items():
-        # If this named member's value names an enum type, then all members
+        # If the discriminator names an enum type, then all members
         # of 'data' must also be members of the enum type.
-        if enum_define and not key in enum_define['enum_values']:
-            raise QAPIExprError(expr_info,
-                                "Discriminator value '%s' is not found in "
-                                "enum '%s'" %
-                                (key, enum_define["enum_name"]))
-        # Todo: add checking for values. Key is checked as above, value can be
-        # also checked here, but we need more functions to handle array case.
+        if enum_define:
+            if not key in enum_define['enum_values']:
+                raise QAPIExprError(expr_info,
+                                    "Discriminator value '%s' is not found in "
+                                    "enum '%s'" %
+                                    (key, enum_define["enum_name"]))
+
+        # Otherwise, check for conflicts in the generated enum
+        else:
+            c_key = _generate_enum_string(key)
+            if c_key in values:
+                raise QAPIExprError(expr_info,
+                                    "Union '%s' member '%s' clashes with '%s'"
+                                    % (name, key, values[c_key]))
+            values[c_key] = key
+
+        # Ensure anonymous unions have no type conflicts.
+        if discriminator == {}:
+            if isinstance(value, list):
+                raise QAPIExprError(expr_info,
+                                    "Anonymous union '%s' member '%s' must "
+                                    "not be array type" % (name, key))
+            qtype = find_anonymous_member_qtype(value)
+            if not qtype:
+                raise QAPIExprError(expr_info,
+                                    "Anonymous union '%s' member '%s' has "
+                                    "invalid type '%s'" % (name, key, value))
+            if qtype in types_seen:
+                raise QAPIExprError(expr_info,
+                                    "Anonymous union '%s' member '%s' can't "
+                                    "be distinguished from member '%s'"
+                                    % (name, key, types_seen[qtype]))
+            types_seen[qtype] = key
+
 
 def check_enum(expr, expr_info):
     name = expr['enum']