qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v6 10/36] qapi: Forbid base without discriminato


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v6 10/36] qapi: Forbid base without discriminator in unions
Date: Mon, 27 Apr 2015 19:36:16 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
>
> 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 <address@hidden>
> @@ -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 <address@hidden>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]