[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 40/40] qapi: Check for member name conflicts
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v8 40/40] qapi: Check for member name conflicts with a base class |
Date: |
Mon, 04 May 2015 20:13:02 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
>
> 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 <address@hidden>
- [Qemu-devel] [PATCH v8 28/40] qapi: Prefer 'struct' over 'type' in generator, (continued)
- [Qemu-devel] [PATCH v8 28/40] qapi: Prefer 'struct' over 'type' in generator, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 36/40] qapi: Drop support for inline nested types, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 34/40] qapi: Drop inline nested struct in query-version, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 32/40] qapi: Merge UserDefTwo and UserDefNested in tests, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 24/40] qapi: More rigourous checking of types, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 37/40] qapi: Drop dead visitor code related to nested structs, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 35/40] qapi: Drop inline nested structs in query-pci, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 40/40] qapi: Check for member name conflicts with a base class, Eric Blake, 2015/05/04
- Re: [Qemu-devel] [PATCH v8 40/40] qapi: Check for member name conflicts with a base class,
Markus Armbruster <=
- [Qemu-devel] [PATCH v8 33/40] qapi: Drop tests for inline nested structs, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 39/40] qapi: Support (subset of) \u escapes in strings, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 30/40] qapi: Use 'struct' instead of 'type' in schema, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 38/40] qapi: Tweak doc references to QMP when QGA is also meant, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 06/40] qapi: Add some enum tests, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 01/40] qapi: Add copyright declaration on docs, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 02/40] qapi: Document type-safety considerations, Eric Blake, 2015/05/04
- [Qemu-devel] [PATCH v8 08/40] qapi: Add some union tests, Eric Blake, 2015/05/04
- Re: [Qemu-devel] [PATCH v8 00/40] drop qapi nested structs, Markus Armbruster, 2015/05/04