Re: [Qemu-devel] [PATCH v5 08/46] qapi: Reuse code for flat union base validation

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Rather than open-code the check for a valid base type, we
> should reuse the common functionality. This allows for
> consistent error messages, and also makes it easier for a
> later patch to turn on support for inline anonymous base
> structures.
>
> Test flat-union-inline is updated to test only one feature
> (anonymous branch dictionaries), which can be implemented
> independently (test flat-union-bad-base already covers the
> idea of an anonymous base dictionary).
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi.py                             | 11 +++++------
>  tests/qapi-schema/flat-union-bad-base.err   |  2 +-
>  tests/qapi-schema/flat-union-base-any.err   |  2 +-
>  tests/qapi-schema/flat-union-base-union.err |  2 +-
>  tests/qapi-schema/flat-union-inline.err     |  2 +-
>  tests/qapi-schema/flat-union-inline.json    |  4 ++--
>  tests/qapi-schema/flat-union-no-base.err    |  2 +-
>  tests/qapi-schema/union-invalid-base.err    |  2 +-
>  8 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 007349e..6f4e471 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -560,15 +560,14 @@ def check_union(expr, expr_info):
>      # Else, it's a flat union.
>      else:
>          # The object must have a string member 'base'.
> -        if not isinstance(base, str):
> +        check_type(expr_info, "'base' for union '%s'" % name,
> +                   base, allow_metas=['struct'])
> +        if not base:
>              raise QAPIExprError(expr_info,
> -                                "Flat union '%s' must have a string base 
> field"
> +                                "Flat union '%s' must have a valid base"

Well, it must have a base, period.  Suggest to drop "valid".

>                                  % name)
>          base_fields = find_base_fields(base)
> -        if not base_fields:
> -            raise QAPIExprError(expr_info,
> -                                "Base '%s' is not a valid struct"
> -                                % base)
> +        assert base_fields
>
>          # The value of member 'discriminator' must name a non-optional
>          # member of the base struct.
> diff --git a/tests/qapi-schema/flat-union-bad-base.err 
> b/tests/qapi-schema/flat-union-bad-base.err
> index f9c31b2..79b8a71 100644
> --- a/tests/qapi-schema/flat-union-bad-base.err
> +++ b/tests/qapi-schema/flat-union-bad-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-bad-base.json:9: Flat union 'TestUnion' must 
> have a string base field
> +tests/qapi-schema/flat-union-bad-base.json:9: 'base' for union 'TestUnion' 
> should be a type name

Improvement.

> diff --git a/tests/qapi-schema/flat-union-base-any.err 
> b/tests/qapi-schema/flat-union-base-any.err
> index ad4d629..646f1c9 100644
> --- a/tests/qapi-schema/flat-union-base-any.err
> +++ b/tests/qapi-schema/flat-union-base-any.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-base-any.json:8: Base 'any' is not a valid 
> struct
> +tests/qapi-schema/flat-union-base-any.json:8: 'base' for union 'TestUnion' 
> cannot use built-in type 'any'

Improvement.

> diff --git a/tests/qapi-schema/flat-union-base-union.err 
> b/tests/qapi-schema/flat-union-base-union.err
> index ede9859..d50e687 100644
> --- a/tests/qapi-schema/flat-union-base-union.err
> +++ b/tests/qapi-schema/flat-union-base-union.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a 
> valid struct
> +tests/qapi-schema/flat-union-base-union.json:11: 'base' for union 
> 'TestUnion' cannot use union type 'UnionBase'

Improvement.

> diff --git a/tests/qapi-schema/flat-union-inline.err 
> b/tests/qapi-schema/flat-union-inline.err
> index ec58627..2333358 100644
> --- a/tests/qapi-schema/flat-union-inline.err
> +++ b/tests/qapi-schema/flat-union-inline.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-inline.json:7: Flat union 'TestUnion' must have 
> a string base field
> +tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 
> 'TestUnion' should be a type name
> diff --git a/tests/qapi-schema/flat-union-inline.json 
> b/tests/qapi-schema/flat-union-inline.json
> index 6bfdd65..62c7cda 100644
> --- a/tests/qapi-schema/flat-union-inline.json
> +++ b/tests/qapi-schema/flat-union-inline.json
> @@ -1,11 +1,11 @@
>  # we require branches to be a struct name
> -# TODO: should we allow anonymous inline types?
> +# TODO: should we allow anonymous inline branch types?
>  { 'enum': 'TestEnum',
>    'data': [ 'value1', 'value2' ] }
>  { 'struct': 'Base',
>    'data': { 'enum1': 'TestEnum', 'kind': 'str' } }
>  { 'union': 'TestUnion',
> -  'base': { 'enum1': 'TestEnum', 'kind': 'str' },
> +  'base': 'Base',
>    'discriminator': 'enum1',
>    'data': { 'value1': { 'string': 'str' },
>              'value2': { 'integer': 'int' } } }

Makes sense.  Thanks for pointing to flat-union-bad-base.json in your
commit message.

> diff --git a/tests/qapi-schema/flat-union-no-base.err 
> b/tests/qapi-schema/flat-union-no-base.err
> index bb3f708..253e251 100644
> --- a/tests/qapi-schema/flat-union-no-base.err
> +++ b/tests/qapi-schema/flat-union-no-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must 
> have a string base field
> +tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must 
> have a valid base
> diff --git a/tests/qapi-schema/union-invalid-base.err 
> b/tests/qapi-schema/union-invalid-base.err
> index 9f63796..03d7b97 100644
> --- a/tests/qapi-schema/union-invalid-base.err
> +++ b/tests/qapi-schema/union-invalid-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-invalid-base.json:8: Base 'int' is not a valid struct
> +tests/qapi-schema/union-invalid-base.json:8: 'base' for union 'TestUnion' 
> cannot use built-in type 'int'

Another improvement.