| Message ID | 1430751937-17523-41-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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: + 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) # 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', 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' } }
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/tests/qapi-schema/struct-base-clash.out b/tests/qapi-schema/struct-base-clash.out new file mode 100644 index 0000000..e69de29