qapi: Forbid base without discriminator in unions

None of the existing QMP or QGA interfaces uses a union with a
base type but no discriminator; it is easier to avoid this in the
generator to save room for other future extensions more likely to
be useful.  An earlier commit added a union-base-no-discriminator
test to ensure that we eventually give a decent error message;
likewise, removing UserDefUnion outright is okay, because we moved
all the tests we wish to keep into the tests of the simple union
UserDefNativeListUnion in the previous commit.  Now is the time to
actually forbid simple union with base, and remove the last
vestiges from the testsuite.

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

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index e400b03479a33b4deabebecffffb0269375f3f49..f6fb93027954a6e71b33081de80a08f19163a48f 100644 (file)
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -242,10 +242,9 @@ struct %(name)s
 ''')
 
     if base:
-        base_fields = find_struct(base)['data']
-        if discriminator:
-            base_fields = base_fields.copy()
-            del base_fields[discriminator]
+        assert discriminator
+        base_fields = find_struct(base)['data'].copy()
+        del base_fields[discriminator]
         ret += generate_struct_fields(base_fields)
     else:
         assert not discriminator

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 41596bb95b20cb551ee213a177a0832cf3382585..dbf0101cba2610e42f77a2dd7d12003071ff2ace 100644 (file)
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -310,16 +310,15 @@ def generate_visit_union(expr):
         ret = ""
         disc_type = enum_define['enum_name']
     else:
-        # There will always be a discriminator in the C switch code, by default it
-        # is an enum type generated silently as "'%sKind' % (name)"
+        # There will always be a discriminator in the C switch code, by default
+        # it is an enum type generated silently as "'%sKind' % (name)"
         ret = generate_visit_enum('%sKind' % name, members.keys())
         disc_type = '%sKind' % (name)
 
     if base:
-        base_fields = find_struct(base)['data']
-        if discriminator:
-            base_fields = base_fields.copy()
-            del base_fields[discriminator]
+        assert discriminator
+        base_fields = find_struct(base)['data'].copy()
+        del base_fields[discriminator]
         ret += generate_visit_struct_fields(name, "", "", base_fields)
 
     if discriminator:

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3ce8c3321b2deb4c9d9fe25112c5a6ab233aac40..438468e3aa4ab36a46fe8923ed1eb46a95647b4a 100644 (file)
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -259,22 +259,22 @@ def check_union(expr, expr_info):
     discriminator = expr.get('discriminator')
     members = expr['data']
 
-    # If the object has a member 'base', its value must name a complex type.
-    if base:
+    # If the object has a member 'base', its value must name a complex type,
+    # and there must be a discriminator.
+    if base is not None:
+        if discriminator is None:
+            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 an
-    # ordinary union.
-    if not discriminator:
-        enum_define = None
-
-    # Else if the value of member 'discriminator' is {}, it's an
-    # anonymous union.
-    elif discriminator == {}:
+    # 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 == {}:
         enum_define = None
 
     # Else, it's a flat union.

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 84f0f0763769c151e214140fca587ca6171c9369..b134f3fa29f7eb9da77d9f4a302f0ba4d2cfd628 100644 (file)
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -36,10 +36,6 @@
 { 'type': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
 
-{ 'union': 'UserDefUnion',
-  'base': 'UserDefZero',
-  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
-
 { 'type': 'UserDefUnionBase',
   'data': { 'string': 'str', 'enum1': 'EnumOne' } }
 

diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 915a61bcd11cc9ec48046010cc4bda9743702cbf..664ae7b4b3b597dc885914c58e66984a185ad408 100644 (file)
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -7,7 +7,6 @@
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
- OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
  OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
  OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
@@ -24,7 +23,6 @@
  OrderedDict([('event', 'EVENT_C'), ('data', OrderedDict([('*a', 'int'), ('*b', 'UserDefOne'), ('c', 'str')]))]),
  OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a', 'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))])]
 [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
- {'enum_name': 'UserDefUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),

diff --git a/tests/qapi-schema/union-base-no-discriminator.err b/tests/qapi-schema/union-base-no-discriminator.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..fc8b79c45965ff50d8988d845a00f80744ad69c4 100644 (file)
--- a/tests/qapi-schema/union-base-no-discriminator.err
+++ b/tests/qapi-schema/union-base-no-discriminator.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-base-no-discriminator.json:11: Union 'TestUnion' requires a discriminator to go along with base

diff --git a/tests/qapi-schema/union-base-no-discriminator.exit b/tests/qapi-schema/union-base-no-discriminator.exit
index 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- a/tests/qapi-schema/union-base-no-discriminator.exit
+++ b/tests/qapi-schema/union-base-no-discriminator.exit
@@ -1 +1 @@
-0
+1

diff --git a/tests/qapi-schema/union-base-no-discriminator.json b/tests/qapi-schema/union-base-no-discriminator.json
index c8cba3ad7c09b32e42142d4a67a90d4cefccf447..052596c46ee1849e321bca9b29b50727447c3ed3 100644 (file)
--- a/tests/qapi-schema/union-base-no-discriminator.json
+++ b/tests/qapi-schema/union-base-no-discriminator.json
@@ -1,4 +1,4 @@
-# FIXME: we should reject simple unions with a base
+# we reject simple unions with a base (or flat unions without discriminator)
 { 'type': 'TestTypeA',
   'data': { 'string': 'str' } }
 

diff --git a/tests/qapi-schema/union-base-no-discriminator.out b/tests/qapi-schema/union-base-no-discriminator.out
index 505fd57dc0ed298856b07215bc78eec2e6b6464c..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/union-base-no-discriminator.out
+++ b/tests/qapi-schema/union-base-no-discriminator.out
@@ -1,8 +0,0 @@
-[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]),
- OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))]),
- OrderedDict([('union', 'TestUnion'), ('base', 'Base'), ('data', OrderedDict([('value1', 'TestTypeA'), ('value2', 'TestTypeB')]))])]
-[{'enum_name': 'TestUnionKind', 'enum_values': None}]
-[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]),
- OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))])]

diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 0039ff6f070009d855cc6d44ef5c4c85775ec42f..cc33f6461df11e0ca354e09718268617351b8b85 100644 (file)
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -293,23 +293,6 @@ static void test_visitor_in_list(TestInputVisitorData *data,
     qapi_free_UserDefOneList(head);
 }
 
-static void test_visitor_in_union(TestInputVisitorData *data,
-                                  const void *unused)
-{
-    Visitor *v;
-    Error *err = NULL;
-    UserDefUnion *tmp;
-
-    v = visitor_input_test_init(data, "{ 'type': 'b', 'integer': 41, 'data' : { 'integer': 42 } }");
-
-    visit_type_UserDefUnion(v, &tmp, NULL, &err);
-    g_assert(err == NULL);
-    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_B);
-    g_assert_cmpint(tmp->integer, ==, 41);
-    g_assert_cmpint(tmp->b->integer, ==, 42);
-    qapi_free_UserDefUnion(tmp);
-}
-
 static void test_visitor_in_union_flat(TestInputVisitorData *data,
                                        const void *unused)
 {
@@ -679,8 +662,6 @@ int main(int argc, char **argv)
                            &in_visitor_data, test_visitor_in_struct_nested);
     input_visitor_test_add("/visitor/input/list",
                            &in_visitor_data, test_visitor_in_list);
-    input_visitor_test_add("/visitor/input/union",
-                            &in_visitor_data, test_visitor_in_union);
     input_visitor_test_add("/visitor/input/union-flat",
                            &in_visitor_data, test_visitor_in_union_flat);
     input_visitor_test_add("/visitor/input/union-anon",

diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index e5bf40fabbf8326b862c7444d8bc039ca64f80bb..ebe6ea3373b2f48406c46a88b3c5909b28158bd4 100644 (file)
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -422,40 +422,6 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
     qapi_free_UserDefNestedList(head);
 }
 
-static void test_visitor_out_union(TestOutputVisitorData *data,
-                                   const void *unused)
-{
-    QObject *arg, *qvalue;
-    QDict *qdict, *value;
-
-    Error *err = NULL;
-
-    UserDefUnion *tmp = g_malloc0(sizeof(UserDefUnion));
-    tmp->kind = USER_DEF_UNION_KIND_A;
-    tmp->integer = 41;
-    tmp->a = g_malloc0(sizeof(UserDefA));
-    tmp->a->boolean = true;
-
-    visit_type_UserDefUnion(data->ov, &tmp, NULL, &err);
-    g_assert(err == NULL);
-    arg = qmp_output_get_qobject(data->qov);
-
-    g_assert(qobject_type(arg) == QTYPE_QDICT);
-    qdict = qobject_to_qdict(arg);
-
-    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "a");
-    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41);
-
-    qvalue = qdict_get(qdict, "data");
-    g_assert(data != NULL);
-    g_assert(qobject_type(qvalue) == QTYPE_QDICT);
-    value = qobject_to_qdict(qvalue);
-    g_assert_cmpint(qdict_get_bool(value, "boolean"), ==, true);
-
-    qapi_free_UserDefUnion(tmp);
-    QDECREF(qdict);
-}
-
 static void test_visitor_out_union_flat(TestOutputVisitorData *data,
                                         const void *unused)
 {
@@ -862,8 +828,6 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/list-qapi-free",
                             &out_visitor_data, test_visitor_out_list_qapi_free);
-    output_visitor_test_add("/visitor/output/union",
-                            &out_visitor_data, test_visitor_out_union);
     output_visitor_test_add("/visitor/output/union-flat",
                             &out_visitor_data, test_visitor_out_union_flat);
     output_visitor_test_add("/visitor/output/union-anon",