| Message ID | 1411165504-18198-7-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The previous commit demonstrated that the generator had several > flaws with less-than-perfect enums: > - an enum that listed the same string twice (or two variant > strings that map to the same C enum) ended up generating an > invalid C enum > - because the generator adds a _MAX terminator to each enum, > the use of an enum member 'max' can also cause this clash > - if an enum omits 'data', the generator left a python stack > trace rather than a graceful message > - an enum used a non-array 'data' was silently accepted by > the parser > - an enum that used non-string members in the 'data' member > was silently accepted by the parser > > Add check_enum to cover these situations, and update testcases > to match. While valid .json files won't trigger any of these > cases, we might as well be nicer to developers that make a typo > while trying to add new QAPI code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > scripts/qapi.py | 32 ++++++++++++++++++++++++++++---- > tests/qapi-schema/enum-clash-member.err | 1 + > tests/qapi-schema/enum-clash-member.exit | 2 +- > tests/qapi-schema/enum-clash-member.json | 2 +- > tests/qapi-schema/enum-clash-member.out | 3 --- > tests/qapi-schema/enum-dict-member.err | 1 + > tests/qapi-schema/enum-dict-member.exit | 2 +- > tests/qapi-schema/enum-dict-member.json | 2 +- > tests/qapi-schema/enum-dict-member.out | 3 --- > tests/qapi-schema/enum-max-member.err | 1 + > tests/qapi-schema/enum-max-member.exit | 2 +- > tests/qapi-schema/enum-max-member.json | 4 ++-- > tests/qapi-schema/enum-max-member.out | 3 --- > tests/qapi-schema/enum-missing-data.err | 7 +------ > tests/qapi-schema/enum-missing-data.json | 2 +- > tests/qapi-schema/enum-wrong-data.err | 1 + > tests/qapi-schema/enum-wrong-data.exit | 2 +- > tests/qapi-schema/enum-wrong-data.json | 2 +- > tests/qapi-schema/enum-wrong-data.out | 3 --- > 19 files changed, 43 insertions(+), 32 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 77d46aa..85aa8bf 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -2,7 +2,7 @@ > # QAPI helper library > # > # Copyright IBM, Corp. 2011 > -# Copyright (c) 2013 Red Hat Inc. > +# Copyright (c) 2013-2014 Red Hat Inc. > # > # Authors: > # Anthony Liguori <aliguori@us.ibm.com> > @@ -316,13 +316,37 @@ def check_union(expr, expr_info): > # Todo: add checking for values. Key is checked as above, value can be > # also checked here, but we need more functions to handle array case. > > +def check_enum(expr, expr_info): > + name = expr['enum'] > + members = expr.get('data') > + values = { 'MAX': '(automatic)' } > + > + if not isinstance(members, list): > + raise QAPIExprError(expr_info, > + "enum '%s' requires an array for 'data'" % name) > + for member in members: > + if not isinstance(member, basestring): > + raise QAPIExprError(expr_info, > + "enum '%s' member '%s' is not a string" > + % (name, member)) > + key = _generate_enum_string(member) > + if key in values: > + raise QAPIExprError(expr_info, > + "enum '%s' member '%s' clashes with '%s'" > + % (name, member, values[key])) > + values[key] = member > + > def check_exprs(schema): > for expr_elem in schema.exprs: > expr = expr_elem['expr'] > + info = expr_elem['info'] > + > if expr.has_key('union'): > - check_union(expr, expr_elem['info']) > + check_union(expr, info) > if expr.has_key('event'): > - check_event(expr, expr_elem['info']) > + check_event(expr, info) > + if expr.has_key('enum'): > + check_enum(expr, info) > > def parse_schema(input_file): > try: > @@ -336,7 +360,7 @@ def parse_schema(input_file): > for expr_elem in schema.exprs: > expr = expr_elem['expr'] > if expr.has_key('enum'): > - add_enum(expr['enum'], expr['data']) > + add_enum(expr['enum'], expr.get('data')) > elif expr.has_key('union'): > add_union(expr) > elif expr.has_key('type'): > diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err > index e69de29..3731fc2 100644 > --- a/tests/qapi-schema/enum-clash-member.err > +++ b/tests/qapi-schema/enum-clash-member.err > @@ -0,0 +1 @@ > +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE' clashes with 'one' > diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-schema/enum-clash-member.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/enum-clash-member.exit > +++ b/tests/qapi-schema/enum-clash-member.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json > index cb4b428..c668ff5 100644 > --- a/tests/qapi-schema/enum-clash-member.json > +++ b/tests/qapi-schema/enum-clash-member.json > @@ -1,2 +1,2 @@ > -# FIXME: we should reject enums where members will clash in C switch > +# we reject enums where members will clash in C switch > { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } Actually in PATCH 05 already: "clash in C switch". I guess you mean we generate an enum with clashing enumeration constants. In the test case, we'd generate MY_ENUM_ONE (I think) both for 'one' and 'ONE'. Correct? > diff --git a/tests/qapi-schema/enum-clash-member.out b/tests/qapi-schema/enum-clash-member.out > index 0814459..e69de29 100644 > --- a/tests/qapi-schema/enum-clash-member.out > +++ b/tests/qapi-schema/enum-clash-member.out > @@ -1,3 +0,0 @@ > -[OrderedDict([('enum', 'MyEnum'), ('data', ['one', 'ONE'])])] > -[{'enum_name': 'MyEnum', 'enum_values': ['one', 'ONE']}] > -[] > diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err > index e69de29..f5a8ffe 100644 > --- a/tests/qapi-schema/enum-dict-member.err > +++ b/tests/qapi-schema/enum-dict-member.err > @@ -0,0 +1 @@ > +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member 'OrderedDict([('value', 'str')])' is not a string Error message is in terms of implementation instead of source. Since this is merely a development tool, it'll do. Same elsewhere, and I'm not going to flag it. Precedents in master quite possible. > diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/enum-dict-member.exit > +++ b/tests/qapi-schema/enum-dict-member.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json > index de4d6bf..79672e0 100644 > --- a/tests/qapi-schema/enum-dict-member.json > +++ b/tests/qapi-schema/enum-dict-member.json > @@ -1,2 +1,2 @@ > -# FIXME: we should reject any enum member that is not a string > +# we reject any enum member that is not a string > { 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] } > diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out > index 8b293f8..e69de29 100644 > --- a/tests/qapi-schema/enum-dict-member.out > +++ b/tests/qapi-schema/enum-dict-member.out > @@ -1,3 +0,0 @@ > -[OrderedDict([('enum', 'MyEnum'), ('data', [OrderedDict([('value', 'str')])])])] > -[{'enum_name': 'MyEnum', 'enum_values': [OrderedDict([('value', 'str')])]}] > -[] > diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err > index e69de29..a6c5db1 100644 > --- a/tests/qapi-schema/enum-max-member.err > +++ b/tests/qapi-schema/enum-max-member.err > @@ -0,0 +1 @@ > +tests/qapi-schema/enum-max-member.json:3: enum 'MyEnum' member 'max' clashes with '(automatic)' > diff --git a/tests/qapi-schema/enum-max-member.exit b/tests/qapi-schema/enum-max-member.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/enum-max-member.exit > +++ b/tests/qapi-schema/enum-max-member.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/enum-max-member.json b/tests/qapi-schema/enum-max-member.json > index ea854c4..6af4662 100644 > --- a/tests/qapi-schema/enum-max-member.json > +++ b/tests/qapi-schema/enum-max-member.json > @@ -1,3 +1,3 @@ > -# FIXME: we should either reject user-supplied 'max', or munge the implicit > -# max value we generate at the end of an array > +# we reject user-supplied 'max' for clashing with implicit enum end > +# FIXME: should we instead munge the the implicit value to avoid the clash? Or pick an identifier for the max member so that it cannot clash with the ones we generate for the user's members. The generator picks identifiers pretty much thoughtlessly in general. If something clashes, the C compiler spits it out, and you get to fiddle with the .json. [...] Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 09/23/2014 08:23 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> The previous commit demonstrated that the generator had several >> flaws with less-than-perfect enums: >> - an enum that listed the same string twice (or two variant >> strings that map to the same C enum) ended up generating an >> invalid C enum >> - because the generator adds a _MAX terminator to each enum, >> the use of an enum member 'max' can also cause this clash >> - if an enum omits 'data', the generator left a python stack >> trace rather than a graceful message >> - an enum used a non-array 'data' was silently accepted by >> the parser >> - an enum that used non-string members in the 'data' member >> was silently accepted by the parser >> >> Add check_enum to cover these situations, and update testcases >> to match. While valid .json files won't trigger any of these >> cases, we might as well be nicer to developers that make a typo >> while trying to add new QAPI code. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> --- a/tests/qapi-schema/enum-clash-member.err >> +++ b/tests/qapi-schema/enum-clash-member.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE' clashes with 'one' >> diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-schema/enum-clash-member.exit >> index 573541a..d00491f 100644 >> --- a/tests/qapi-schema/enum-clash-member.exit >> +++ b/tests/qapi-schema/enum-clash-member.exit >> @@ -1 +1 @@ >> -0 >> +1 >> diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json >> index cb4b428..c668ff5 100644 >> --- a/tests/qapi-schema/enum-clash-member.json >> +++ b/tests/qapi-schema/enum-clash-member.json >> @@ -1,2 +1,2 @@ >> -# FIXME: we should reject enums where members will clash in C switch >> +# we reject enums where members will clash in C switch >> { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } > > Actually in PATCH 05 already: "clash in C switch". I guess you mean we > generate an enum with clashing enumeration constants. In the test case, > we'd generate MY_ENUM_ONE (I think) both for 'one' and 'ONE'. Correct? Correct; the generated C code would include an invalid switch statement with two repetitions of the same spelling of a case label. In patch 5, the test case demonstrates that the parser was silently accepting code that resulted in a clash in the generated C code; this patch updates both qapi.py to make it a hard error, and the testsuite to change from (accidental) pass to (intentional) error detection, so that we no longer have to worry about the issue in the generated C code. The same principle applies throughout my series - I first introduced new tests in isolation for existing pre-patch behavior, with FIXME comments where the tests expose bogus behavior, then in later patches fix the parser to reject bogus behavior and update the test to match that it now covers the new error message. >> +++ b/tests/qapi-schema/enum-dict-member.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member 'OrderedDict([('value', 'str')])' is not a string > > Error message is in terms of implementation instead of source. Since > this is merely a development tool, it'll do. Same elsewhere, and I'm > not going to flag it. Precedents in master quite possible. Yeah, I couldn't figure a way to get back at the original text typed by the user. I'm open to suggestions, but I'm (obviously) okay with leaving it as is. >> +++ b/tests/qapi-schema/enum-max-member.json >> @@ -1,3 +1,3 @@ >> -# FIXME: we should either reject user-supplied 'max', or munge the implicit >> -# max value we generate at the end of an array >> +# we reject user-supplied 'max' for clashing with implicit enum end >> +# FIXME: should we instead munge the the implicit value to avoid the clash? > > Or pick an identifier for the max member so that it cannot clash with > the ones we generate for the user's members. > > The generator picks identifiers pretty much thoughtlessly in general. > If something clashes, the C compiler spits it out, and you get to fiddle > with the .json. Well, hopefully by hoisting the error message away from C compilation time (late) to qapi.py runtime (early), we have made it easier for anyone actually needing the name 'max' to identify what still needs fixing. At any rate, I'm always a fan of erroring out as early as possible, rather than waiting for an obscure crash later on in the C compilation :) > > [...] > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks.
Eric Blake <eblake@redhat.com> writes: > On 09/23/2014 08:23 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> The previous commit demonstrated that the generator had several >>> flaws with less-than-perfect enums: >>> - an enum that listed the same string twice (or two variant >>> strings that map to the same C enum) ended up generating an >>> invalid C enum >>> - because the generator adds a _MAX terminator to each enum, >>> the use of an enum member 'max' can also cause this clash >>> - if an enum omits 'data', the generator left a python stack >>> trace rather than a graceful message >>> - an enum used a non-array 'data' was silently accepted by >>> the parser >>> - an enum that used non-string members in the 'data' member >>> was silently accepted by the parser >>> >>> Add check_enum to cover these situations, and update testcases >>> to match. While valid .json files won't trigger any of these >>> cases, we might as well be nicer to developers that make a typo >>> while trying to add new QAPI code. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- > >>> --- a/tests/qapi-schema/enum-clash-member.err >>> +++ b/tests/qapi-schema/enum-clash-member.err >>> @@ -0,0 +1 @@ >>> +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE' clashes with 'one' >>> diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-schema/enum-clash-member.exit >>> index 573541a..d00491f 100644 >>> --- a/tests/qapi-schema/enum-clash-member.exit >>> +++ b/tests/qapi-schema/enum-clash-member.exit >>> @@ -1 +1 @@ >>> -0 >>> +1 >>> diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json >>> index cb4b428..c668ff5 100644 >>> --- a/tests/qapi-schema/enum-clash-member.json >>> +++ b/tests/qapi-schema/enum-clash-member.json >>> @@ -1,2 +1,2 @@ >>> -# FIXME: we should reject enums where members will clash in C switch >>> +# we reject enums where members will clash in C switch >>> { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } >> >> Actually in PATCH 05 already: "clash in C switch". I guess you mean we >> generate an enum with clashing enumeration constants. In the test case, >> we'd generate MY_ENUM_ONE (I think) both for 'one' and 'ONE'. Correct? > > Correct; the generated C code would include an invalid switch statement > with two repetitions of the same spelling of a case label. Actually, it generates a broken enum definition. For instance, { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } produces typedef enum BadEnum { BAD_ENUM_ONE = 0, BAD_ENUM_ONE = 1, BAD_ENUM_MAX = 2, } BadEnum; Drop "in C switch" from the comment? > In patch 5, > the test case demonstrates that the parser was silently accepting code > that resulted in a clash in the generated C code; this patch updates > both qapi.py to make it a hard error, and the testsuite to change from > (accidental) pass to (intentional) error detection, so that we no longer > have to worry about the issue in the generated C code. > > The same principle applies throughout my series - I first introduced new > tests in isolation for existing pre-patch behavior, with FIXME comments > where the tests expose bogus behavior, then in later patches fix the > parser to reject bogus behavior and update the test to match that it now > covers the new error message. That's exactly how I like it done. >>> +++ b/tests/qapi-schema/enum-dict-member.err >>> @@ -0,0 +1 @@ >>> +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member >>> OrderedDict([('value', 'str')])' is not a string >> >> Error message is in terms of implementation instead of source. Since >> this is merely a development tool, it'll do. Same elsewhere, and I'm >> not going to flag it. Precedents in master quite possible. > > Yeah, I couldn't figure a way to get back at the original text typed by > the user. I'm open to suggestions, but I'm (obviously) okay with > leaving it as is. Let's leave it as is for now. >>> +++ b/tests/qapi-schema/enum-max-member.json >>> @@ -1,3 +1,3 @@ >>> -# FIXME: we should either reject user-supplied 'max', or munge the implicit >>> -# max value we generate at the end of an array >>> +# we reject user-supplied 'max' for clashing with implicit enum end >>> +# FIXME: should we instead munge the the implicit value to avoid the clash? >> >> Or pick an identifier for the max member so that it cannot clash with >> the ones we generate for the user's members. >> >> The generator picks identifiers pretty much thoughtlessly in general. >> If something clashes, the C compiler spits it out, and you get to fiddle >> with the .json. > > Well, hopefully by hoisting the error message away from C compilation > time (late) to qapi.py runtime (early), we have made it easier for > anyone actually needing the name 'max' to identify what still needs > fixing. At any rate, I'm always a fan of erroring out as early as > possible, rather than waiting for an obscure crash later on in the C > compilation :) One saturday of happy hacking, followed by years of chipping away at the mess... >> [...] >> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Thanks.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 77d46aa..85aa8bf 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -2,7 +2,7 @@ # QAPI helper library # # Copyright IBM, Corp. 2011 -# Copyright (c) 2013 Red Hat Inc. +# Copyright (c) 2013-2014 Red Hat Inc. # # Authors: # Anthony Liguori <aliguori@us.ibm.com> @@ -316,13 +316,37 @@ def check_union(expr, expr_info): # Todo: add checking for values. Key is checked as above, value can be # also checked here, but we need more functions to handle array case. +def check_enum(expr, expr_info): + name = expr['enum'] + members = expr.get('data') + values = { 'MAX': '(automatic)' } + + if not isinstance(members, list): + raise QAPIExprError(expr_info, + "enum '%s' requires an array for 'data'" % name) + for member in members: + if not isinstance(member, basestring): + raise QAPIExprError(expr_info, + "enum '%s' member '%s' is not a string" + % (name, member)) + key = _generate_enum_string(member) + if key in values: + raise QAPIExprError(expr_info, + "enum '%s' member '%s' clashes with '%s'" + % (name, member, values[key])) + values[key] = member + def check_exprs(schema): for expr_elem in schema.exprs: expr = expr_elem['expr'] + info = expr_elem['info'] + if expr.has_key('union'): - check_union(expr, expr_elem['info']) + check_union(expr, info) if expr.has_key('event'): - check_event(expr, expr_elem['info']) + check_event(expr, info) + if expr.has_key('enum'): + check_enum(expr, info) def parse_schema(input_file): try: @@ -336,7 +360,7 @@ def parse_schema(input_file): for expr_elem in schema.exprs: expr = expr_elem['expr'] if expr.has_key('enum'): - add_enum(expr['enum'], expr['data']) + add_enum(expr['enum'], expr.get('data')) elif expr.has_key('union'): add_union(expr) elif expr.has_key('type'): diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err index e69de29..3731fc2 100644 --- a/tests/qapi-schema/enum-clash-member.err +++ b/tests/qapi-schema/enum-clash-member.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE' clashes with 'one' diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-schema/enum-clash-member.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-clash-member.exit +++ b/tests/qapi-schema/enum-clash-member.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json index cb4b428..c668ff5 100644 --- a/tests/qapi-schema/enum-clash-member.json +++ b/tests/qapi-schema/enum-clash-member.json @@ -1,2 +1,2 @@ -# FIXME: we should reject enums where members will clash in C switch +# we reject enums where members will clash in C switch { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } diff --git a/tests/qapi-schema/enum-clash-member.out b/tests/qapi-schema/enum-clash-member.out index 0814459..e69de29 100644 --- a/tests/qapi-schema/enum-clash-member.out +++ b/tests/qapi-schema/enum-clash-member.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', ['one', 'ONE'])])] -[{'enum_name': 'MyEnum', 'enum_values': ['one', 'ONE']}] -[] diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err index e69de29..f5a8ffe 100644 --- a/tests/qapi-schema/enum-dict-member.err +++ b/tests/qapi-schema/enum-dict-member.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member 'OrderedDict([('value', 'str')])' is not a string diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-dict-member.exit +++ b/tests/qapi-schema/enum-dict-member.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json index de4d6bf..79672e0 100644 --- a/tests/qapi-schema/enum-dict-member.json +++ b/tests/qapi-schema/enum-dict-member.json @@ -1,2 +1,2 @@ -# FIXME: we should reject any enum member that is not a string +# we reject any enum member that is not a string { 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] } diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out index 8b293f8..e69de29 100644 --- a/tests/qapi-schema/enum-dict-member.out +++ b/tests/qapi-schema/enum-dict-member.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', [OrderedDict([('value', 'str')])])])] -[{'enum_name': 'MyEnum', 'enum_values': [OrderedDict([('value', 'str')])]}] -[] diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err index e69de29..a6c5db1 100644 --- a/tests/qapi-schema/enum-max-member.err +++ b/tests/qapi-schema/enum-max-member.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-max-member.json:3: enum 'MyEnum' member 'max' clashes with '(automatic)' diff --git a/tests/qapi-schema/enum-max-member.exit b/tests/qapi-schema/enum-max-member.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-max-member.exit +++ b/tests/qapi-schema/enum-max-member.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-max-member.json b/tests/qapi-schema/enum-max-member.json index ea854c4..6af4662 100644 --- a/tests/qapi-schema/enum-max-member.json +++ b/tests/qapi-schema/enum-max-member.json @@ -1,3 +1,3 @@ -# FIXME: we should either reject user-supplied 'max', or munge the implicit -# max value we generate at the end of an array +# we reject user-supplied 'max' for clashing with implicit enum end +# FIXME: should we instead munge the the implicit value to avoid the clash? { 'enum': 'MyEnum', 'data': [ 'max' ] } diff --git a/tests/qapi-schema/enum-max-member.out b/tests/qapi-schema/enum-max-member.out index c933044..e69de29 100644 --- a/tests/qapi-schema/enum-max-member.out +++ b/tests/qapi-schema/enum-max-member.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', ['max'])])] -[{'enum_name': 'MyEnum', 'enum_values': ['max']}] -[] diff --git a/tests/qapi-schema/enum-missing-data.err b/tests/qapi-schema/enum-missing-data.err index 1fec213..4b8b59e 100644 --- a/tests/qapi-schema/enum-missing-data.err +++ b/tests/qapi-schema/enum-missing-data.err @@ -1,6 +1 @@ -Traceback (most recent call last): - File "tests/qapi-schema/test-qapi.py", line 19, in <module> - exprs = parse_schema(sys.argv[1]) - File "scripts/qapi.py", line 339, in parse_schema - add_enum(expr['enum'], expr['data']) -KeyError: 'data' +tests/qapi-schema/enum-missing-data.json:2: enum 'MyEnum' requires an array for 'data' diff --git a/tests/qapi-schema/enum-missing-data.json b/tests/qapi-schema/enum-missing-data.json index 01f3f32..558fd35 100644 --- a/tests/qapi-schema/enum-missing-data.json +++ b/tests/qapi-schema/enum-missing-data.json @@ -1,2 +1,2 @@ -# FIXME: we should require that all QAPI enums have a data array +# we require that all QAPI enums have a data array { 'enum': 'MyEnum' } diff --git a/tests/qapi-schema/enum-wrong-data.err b/tests/qapi-schema/enum-wrong-data.err index e69de29..0d14e80 100644 --- a/tests/qapi-schema/enum-wrong-data.err +++ b/tests/qapi-schema/enum-wrong-data.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-wrong-data.json:2: enum 'MyEnum' requires an array for 'data' diff --git a/tests/qapi-schema/enum-wrong-data.exit b/tests/qapi-schema/enum-wrong-data.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-wrong-data.exit +++ b/tests/qapi-schema/enum-wrong-data.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-wrong-data.json b/tests/qapi-schema/enum-wrong-data.json index 61d25ec..7b3e255 100644 --- a/tests/qapi-schema/enum-wrong-data.json +++ b/tests/qapi-schema/enum-wrong-data.json @@ -1,2 +1,2 @@ -# FIXME: we should require that all qapi enums have an array for data +# we require that all qapi enums have an array for data { 'enum': 'MyEnum', 'data': { 'value': 'str' } } diff --git a/tests/qapi-schema/enum-wrong-data.out b/tests/qapi-schema/enum-wrong-data.out index 28d2211..e69de29 100644 --- a/tests/qapi-schema/enum-wrong-data.out +++ b/tests/qapi-schema/enum-wrong-data.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', OrderedDict([('value', 'str')]))])] -[{'enum_name': 'MyEnum', 'enum_values': OrderedDict([('value', 'str')])}] -[]
The previous commit demonstrated that the generator had several flaws with less-than-perfect enums: - an enum that listed the same string twice (or two variant strings that map to the same C enum) ended up generating an invalid C enum - because the generator adds a _MAX terminator to each enum, the use of an enum member 'max' can also cause this clash - if an enum omits 'data', the generator left a python stack trace rather than a graceful message - an enum used a non-array 'data' was silently accepted by the parser - an enum that used non-string members in the 'data' member was silently accepted by the parser Add check_enum to cover these situations, and update testcases to match. While valid .json files won't trigger any of these cases, we might as well be nicer to developers that make a typo while trying to add new QAPI code. Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/qapi.py | 32 ++++++++++++++++++++++++++++---- tests/qapi-schema/enum-clash-member.err | 1 + tests/qapi-schema/enum-clash-member.exit | 2 +- tests/qapi-schema/enum-clash-member.json | 2 +- tests/qapi-schema/enum-clash-member.out | 3 --- tests/qapi-schema/enum-dict-member.err | 1 + tests/qapi-schema/enum-dict-member.exit | 2 +- tests/qapi-schema/enum-dict-member.json | 2 +- tests/qapi-schema/enum-dict-member.out | 3 --- tests/qapi-schema/enum-max-member.err | 1 + tests/qapi-schema/enum-max-member.exit | 2 +- tests/qapi-schema/enum-max-member.json | 4 ++-- tests/qapi-schema/enum-max-member.out | 3 --- tests/qapi-schema/enum-missing-data.err | 7 +------ tests/qapi-schema/enum-missing-data.json | 2 +- tests/qapi-schema/enum-wrong-data.err | 1 + tests/qapi-schema/enum-wrong-data.exit | 2 +- tests/qapi-schema/enum-wrong-data.json | 2 +- tests/qapi-schema/enum-wrong-data.out | 3 --- 19 files changed, 43 insertions(+), 32 deletions(-)