diff mbox

[v6,10/36] qapi: Forbid base without discriminator in unions

Message ID 1428206887-7921-11-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake April 5, 2015, 4:07 a.m. UTC 
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.  A previous commit added a union-base-no-discriminator
test to ensure that we eventually give a decent error message;
now is the time to actually forbid it, and remove the last
vestiges of that usage from the testsuite.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

v6: split from v5:7/28; change subject line; hoist hunk from
v5:8/28 into here to make this be the patch that forbids
simple-with-base
---
 scripts/qapi-types.py                              |  7 ++---
 scripts/qapi-visit.py                              | 13 ++++----
 scripts/qapi.py                                    | 20 ++++++------
 tests/qapi-schema/qapi-schema-test.json            |  4 ---
 tests/qapi-schema/qapi-schema-test.out             |  2 --
 tests/qapi-schema/union-base-no-discriminator.err  |  1 +
 tests/qapi-schema/union-base-no-discriminator.exit |  2 +-
 tests/qapi-schema/union-base-no-discriminator.json |  2 +-
 tests/qapi-schema/union-base-no-discriminator.out  |  8 -----
 tests/test-qmp-input-visitor.c                     | 19 ------------
 tests/test-qmp-output-visitor.c                    | 36 ----------------------
 11 files changed, 22 insertions(+), 92 deletions(-)

Comments

Markus Armbruster April 27, 2015, 5:36 p.m. UTC | #1 
Eric Blake <eblake@redhat.com> writes:

> 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.  A previous commit added a union-base-no-discriminator
> test to ensure that we eventually give a decent error message;
> now is the time to actually forbid it, and remove the last
> vestiges of that usage from the testsuite.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
>
> v6: split from v5:7/28; change subject line; hoist hunk from
> v5:8/28 into here to make this be the patch that forbids
> simple-with-base
> ---
>  scripts/qapi-types.py                              |  7 ++---
>  scripts/qapi-visit.py                              | 13 ++++----
>  scripts/qapi.py                                    | 20 ++++++------
>  tests/qapi-schema/qapi-schema-test.json            |  4 ---
>  tests/qapi-schema/qapi-schema-test.out             |  2 --
>  tests/qapi-schema/union-base-no-discriminator.err  |  1 +
>  tests/qapi-schema/union-base-no-discriminator.exit |  2 +-
>  tests/qapi-schema/union-base-no-discriminator.json |  2 +-
>  tests/qapi-schema/union-base-no-discriminator.out  |  8 -----
>  tests/test-qmp-input-visitor.c                     | 19 ------------
>  tests/test-qmp-output-visitor.c                    | 36 ----------------------
>  11 files changed, 22 insertions(+), 92 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index e400b03..f6fb930 100644
> --- 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 4416677..3f82bd4 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -2,7 +2,7 @@
>  # QAPI visitor generator
>  #
>  # Copyright IBM, Corp. 2011
> -# Copyright (C) 2014 Red Hat, Inc.
> +# Copyright (C) 2014-2015 Red Hat, Inc.
>  #
>  # Authors:
>  #  Anthony Liguori <aliguori@us.ibm.com>
> @@ -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 3ce8c33..438468e 100644
> --- 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 84f0f07..b134f3f 100644
> --- 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' } }
>

Removing UserDefUnion outright is okay, because we still have another
simple union, namely UserDefNativeListUnion, and the previous patch
moved the tests we want to keep from UserDefUnion to
UserDefNativeListUnion.

[...]

Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff mbox

Patch

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index e400b03..f6fb930 100644
--- 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 4416677..3f82bd4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -2,7 +2,7 @@ 
 # QAPI visitor generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (C) 2014 Red Hat, Inc.
+# Copyright (C) 2014-2015 Red Hat, Inc.
 #
 # Authors:
 #  Anthony Liguori <aliguori@us.ibm.com>
@@ -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 3ce8c33..438468e 100644
--- 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 84f0f07..b134f3f 100644
--- 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 915a61b..664ae7b 100644
--- 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 e69de29..fc8b79c 100644
--- 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 573541a..d00491f 100644
--- 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 c8cba3a..052596c 100644
--- 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 505fd57..e69de29 100644
--- 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 0039ff6..cc33f64 100644
--- 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 e5bf40f..ebe6ea3 100644
--- 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",