Re: [Qemu-devel] [PATCH v8 40/40] qapi: Check for member name conflicts with a base class

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Our type inheritance for both 'struct' and for flat 'union' merges
> key/value pairs from the base class with those from the type in
> question.  Although the C code currently boxes things so that there
> is a distinction between which member is referred to, the QMP wire
> format does not allow passing a key more than once in a single
> object.  Besides, if we ever change the generated C code to not be
> quite so boxy, we'd want to avoid duplicate member names there,
> too.
>
> Fix a testsuite entry added in an earlier patch, as well as adding
> a couple more tests to ensure we have appropriate coverage.  Ensure
> that collisions are detected, regardless of whether there is a
> difference in opinion on whether the member name is optional.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
>
> v7: rebase to earlier changes, reuse base instead of expr['base']
> v8: dectect collision in mis-matched optional names (drop R-b)
> ---
>  scripts/qapi.py                                | 23 ++++++++++++++++++++++-
>  tests/Makefile                                 |  3 ++-
>  tests/qapi-schema/flat-union-branch-clash.err  |  1 +
>  tests/qapi-schema/flat-union-branch-clash.exit |  2 +-
>  tests/qapi-schema/flat-union-branch-clash.json |  4 ++--
>  tests/qapi-schema/flat-union-branch-clash.out  |  9 ---------
>  tests/qapi-schema/struct-base-clash-deep.err   |  1 +
>  tests/qapi-schema/struct-base-clash-deep.exit  |  1 +
>  tests/qapi-schema/struct-base-clash-deep.json  |  9 +++++++++
>  tests/qapi-schema/struct-base-clash-deep.out   |  0
>  tests/qapi-schema/struct-base-clash.err        |  1 +
>  tests/qapi-schema/struct-base-clash.exit       |  1 +
>  tests/qapi-schema/struct-base-clash.json       |  6 ++++++
>  tests/qapi-schema/struct-base-clash.out        |  0
>  14 files changed, 47 insertions(+), 14 deletions(-)
>  create mode 100644 tests/qapi-schema/struct-base-clash-deep.err
>  create mode 100644 tests/qapi-schema/struct-base-clash-deep.exit
>  create mode 100644 tests/qapi-schema/struct-base-clash-deep.json
>  create mode 100644 tests/qapi-schema/struct-base-clash-deep.out
>  create mode 100644 tests/qapi-schema/struct-base-clash.err
>  create mode 100644 tests/qapi-schema/struct-base-clash.exit
>  create mode 100644 tests/qapi-schema/struct-base-clash.json
>  create mode 100644 tests/qapi-schema/struct-base-clash.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 14468ba..edfaf9e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -414,6 +414,20 @@ def check_type(expr_info, source, value, allow_array = False,
>                     allow_metas=['built-in', 'union', 'alternate', 'struct',
>                                  'enum'])
>
> +def check_member_clash(expr_info, base_name, data, source = ""):
> +    base = find_struct(base_name)
> +    assert base
> +    base_members = base['data']
> +    for key in data.keys():
> +        if key.startswith('*'):
> +            key = key[1:]
> +        if key in base_members or "*%s" %key in base_members:

Either

    "*%s" % key

or

     "*" + key

Could be fixed up on commit.

> +            raise QAPIExprError(expr_info,
> +                                "Member name '%s'%s clashes with base '%s'"
> +                                %(key, source, base_name))
> +    if base.get('base'):
> +        check_member_clash(expr_info, base['base'], data, source)
> +
>  def check_command(expr, expr_info):
>      name = expr['command']
>      allow_star = expr.has_key('gen')
> @@ -503,9 +517,14 @@ def check_union(expr, expr_info):
>          check_name(expr_info, "Member of union '%s'" % name, key)
>
>          # Each value must name a known type; furthermore, in flat unions,
> -        # branches must be a struct
> +        # branches must be a struct with no overlapping member names
>          check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
>                     value, allow_array=True, allow_metas=allow_metas)
> +        if base:
> +            branch_struct = find_struct(value)
> +            assert branch_struct
> +            check_member_clash(expr_info, base, branch_struct['data'],
> +                               " of branch '%s'" %key)

Likewise.

>
>          # If the discriminator names an enum type, then all members
>          # of 'data' must also be members of the enum type.
> @@ -582,6 +601,8 @@ def check_struct(expr, expr_info):
>                 allow_dict=True, allow_optional=True)
>      check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
>                 allow_metas=['struct'])
> +    if expr.get('base'):
> +        check_member_clash(expr_info, expr['base'], expr['data'])
>
>  def check_exprs(schema):
>      for expr_elem in schema.exprs:
> diff --git a/tests/Makefile b/tests/Makefile
> index 547a249..666aee2 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -243,7 +243,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>  	include-simple.json include-relpath.json include-format-err.json \
>  	include-non-file.json include-no-file.json include-before-err.json \
>  	include-nested-err.json include-self-cycle.json include-cycle.json \
> -	include-repetition.json event-nest-struct.json event-case.json)
> +	include-repetition.json event-nest-struct.json event-case.json \
> +	struct-base-clash.json struct-base-clash-deep.json )
>
>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \
>  		     tests/test-qmp-commands.h tests/test-qapi-event.h
> diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-clash.err
> index e69de29..f112766 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.err
> +++ b/tests/qapi-schema/flat-union-branch-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-branch-clash.exit b/tests/qapi-schema/flat-union-branch-clash.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.exit
> +++ b/tests/qapi-schema/flat-union-branch-clash.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-branch-clash.json
> index 8b0b807..8fb054f 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.json
> +++ b/tests/qapi-schema/flat-union-branch-clash.json
> @@ -1,8 +1,8 @@
> -# FIXME: we should check for no duplicate keys between branches and base
> +# we check for no duplicate keys between branches and base
>  { 'enum': 'TestEnum',
>    'data': [ 'value1', 'value2' ] }
>  { 'struct': 'Base',
> -  'data': { 'enum1': 'TestEnum', 'name': 'str' } }
> +  'data': { 'enum1': 'TestEnum', '*name': 'str' } }
>  { 'struct': 'Branch1',
>    'data': { 'name': 'str' } }
>  { 'struct': 'Branch2',

Amends the test added in PATCH 08.  Perhaps the amend could be squashed
in there.  *Not* worth a respin.

> diff --git a/tests/qapi-schema/flat-union-branch-clash.out b/tests/qapi-schema/flat-union-branch-clash.out
> index 04c2395..e69de29 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.out
> +++ b/tests/qapi-schema/flat-union-branch-clash.out
> @@ -1,9 +0,0 @@
> -[OrderedDict([('enum', 'TestEnum'), ('data', ['value1', 'value2'])]),
> - OrderedDict([('struct', 'Base'), ('data', OrderedDict([('enum1', 'TestEnum'), ('name', 'str')]))]),
> - OrderedDict([('struct', 'Branch1'), ('data', OrderedDict([('name', 'str')]))]),
> - OrderedDict([('struct', 'Branch2'), ('data', OrderedDict([('value', 'int')]))]),
> - OrderedDict([('union', 'TestUnion'), ('base', 'Base'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'Branch1'), ('value2', 'Branch2')]))])]
> -[{'enum_name': 'TestEnum', 'enum_values': ['value1', 'value2']}]
> -[OrderedDict([('struct', 'Base'), ('data', OrderedDict([('enum1', 'TestEnum'), ('name', 'str')]))]),
> - OrderedDict([('struct', 'Branch1'), ('data', OrderedDict([('name', 'str')]))]),
> - OrderedDict([('struct', 'Branch2'), ('data', OrderedDict([('value', 'int')]))])]
> diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
> new file mode 100644
> index 0000000..e3e9f8d
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-deep.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/struct-base-clash-deep.json:7: Member name 'name' clashes with base 'Base'
> diff --git a/tests/qapi-schema/struct-base-clash-deep.exit b/tests/qapi-schema/struct-base-clash-deep.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-deep.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/struct-base-clash-deep.json b/tests/qapi-schema/struct-base-clash-deep.json
> new file mode 100644
> index 0000000..552fe94
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-deep.json
> @@ -0,0 +1,9 @@
> +# we check for no duplicate keys with indirect base
> +{ 'struct': 'Base',
> +  'data': { 'name': 'str' } }
> +{ 'struct': 'Mid',
> +  'base': 'Base',
> +  'data': { 'value': 'int' } }
> +{ 'struct': 'Sub',
> +  'base': 'Mid',
> +  'data': { '*name': 'str' } }
> diff --git a/tests/qapi-schema/struct-base-clash-deep.out b/tests/qapi-schema/struct-base-clash-deep.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
> new file mode 100644
> index 0000000..3ac37fb
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/struct-base-clash.json:4: Member name 'name' clashes with base 'Base'
> diff --git a/tests/qapi-schema/struct-base-clash.exit b/tests/qapi-schema/struct-base-clash.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/struct-base-clash.json b/tests/qapi-schema/struct-base-clash.json
> new file mode 100644
> index 0000000..f2afc9b
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash.json
> @@ -0,0 +1,6 @@
> +# we check for no duplicate keys with base
> +{ 'struct': 'Base',
> +  'data': { 'name': 'str' } }
> +{ 'struct': 'Sub',
> +  'base': 'Base',
> +  'data': { 'name': 'str' } }
> diff --git a/tests/qapi-schema/struct-base-clash.out b/tests/qapi-schema/struct-base-clash.out
> new file mode 100644
> index 0000000..e69de29

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