Eric Blake <ebl...@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 <ebl...@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 <aligu...@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 <arm...@redhat.com>
