Re: [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The previous commit added two tests that triggered an assertion
> failure. It's fairly straightforward to avoid the failure by
> just outright forbidding the collision between a union's tag
> values and its discriminator name (including the implicit name
> 'kind' supplied for simple unions [*]).  Ultimately, we'd like
> to move the collision detection into QAPISchema*.check(), but
> for now it is easier just to enhance the existing checks.
>
> [*] Of course, down the road, we have plans to rename the simple
> union tag name to 'type' to match the QMP wire name, but the
> idea of the collision will still be present even then.
>
> Technically, we could avoid the collision by naming the C union
> members representing each enum value as '_case_value' rather
> than 'value'; but until we have an actual qapi client (and not
> just our testsuite) that has a legitimate reason to match a
> case label to the name of a QMP key and needs the name munging
> to satisfy the compiler, it's easier to just reject the qapi
> as invalid.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v6: new patch
> ---
>  scripts/qapi.py                             | 10 ++++++++--
>  tests/qapi-schema/flat-union-clash-type.err | 17 +----------------
>  tests/qapi-schema/union-clash-type.err      | 17 +----------------
>  3 files changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7e8a99d..ec450a6 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -544,7 +544,7 @@ def check_union(expr, expr_info):
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
>      members = expr['data']
> -    values = {'MAX': '(automatic)'}
> +    values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
>
>      # Two types of unions, determined by discriminator.
>
> @@ -603,13 +603,19 @@ def check_union(expr, expr_info):
>                                 " of branch '%s'" % key)
>
>          # If the discriminator names an enum type, then all members
> -        # of 'data' must also be members of the enum type.
> +        # of 'data' must also be members of the enum type, which in turn
> +        # must not collide with the discriminator name.
>          if enum_define:
>              if key not in enum_define['enum_values']:
>                  raise QAPIExprError(expr_info,
>                                      "Discriminator value '%s' is not found 
> in "
>                                      "enum '%s'" %
>                                      (key, enum_define["enum_name"]))
> +            if discriminator in enum_define['enum_values']:
> +                raise QAPIExprError(expr_info,
> +                                    "Discriminator name '%s' collides with "
> +                                    "enum value in '%s'" %
> +                                    (discriminator, 
> enum_define["enum_name"]))
>
>          # Otherwise, check for conflicts in the generated enum
>          else:
> diff --git a/tests/qapi-schema/flat-union-clash-type.err 
> b/tests/qapi-schema/flat-union-clash-type.err
> index 6e64d1d..879beb8 100644
> --- a/tests/qapi-schema/flat-union-clash-type.err
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -1,16 +1 @@
> -Traceback (most recent call last):
> -  File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> -    schema = QAPISchema(sys.argv[1])
> -  File "scripts/qapi.py", line 1116, in __init__
> -    self.check()
> -  File "scripts/qapi.py", line 1299, in check
> -    ent.check(self)
> -  File "scripts/qapi.py", line 962, in check
> -    self.variants.check(schema, members, seen)
> -  File "scripts/qapi.py", line 1024, in check
> -    v.check(schema, self.tag_member.type, vseen)
> -  File "scripts/qapi.py", line 1032, in check
> -    QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> -  File "scripts/qapi.py", line 994, in check
> -    assert self.name not in seen
> -AssertionError
> +tests/qapi-schema/flat-union-clash-type.json:8: Discriminator name 'type' 
> collides with enum value in 'TestEnum'
> diff --git a/tests/qapi-schema/union-clash-type.err 
> b/tests/qapi-schema/union-clash-type.err
> index 6e64d1d..773900b 100644
> --- a/tests/qapi-schema/union-clash-type.err
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -1,16 +1 @@
> -Traceback (most recent call last):
> -  File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> -    schema = QAPISchema(sys.argv[1])
> -  File "scripts/qapi.py", line 1116, in __init__
> -    self.check()
> -  File "scripts/qapi.py", line 1299, in check
> -    ent.check(self)
> -  File "scripts/qapi.py", line 962, in check
> -    self.variants.check(schema, members, seen)
> -  File "scripts/qapi.py", line 1024, in check
> -    v.check(schema, self.tag_member.type, vseen)
> -  File "scripts/qapi.py", line 1032, in check
> -    QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> -  File "scripts/qapi.py", line 994, in check
> -    assert self.name not in seen
> -AssertionError
> +tests/qapi-schema/union-clash-type.json:2: Union 'TestUnion' member 'kind' 
> clashes with '(automatic)'

Preexisting: this error message sucks.  But improving it isn't this
patch's job.