| Message ID | 1411165504-18198-9-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The previous commit demonstrated that the generator overlooked some > fairly basic broken expressions: > - missing metataype > - metatype key has a non-string value > - unknown key in relation to the metatype > - conflicting metatype (this patch treats the second metatype as an > unknown key of the first key visited, which is not necessarily the > first key the user typed) The whole thing is a Saturday afternoon hack, with extra hacks bolted on left & right. > Add check_keys to cover these situations, and update testcases to > match. A couple other tests (enum-missing-data, indented-expr) had > to change since the validation added here occurs so early. > > 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 | 76 +++++++++++++++++++++++++-------- > tests/qapi-schema/bad-type-dict.err | 1 + > tests/qapi-schema/bad-type-dict.exit | 2 +- > tests/qapi-schema/bad-type-dict.json | 2 +- > tests/qapi-schema/bad-type-dict.out | 3 -- > tests/qapi-schema/double-type.err | 1 + > tests/qapi-schema/double-type.exit | 2 +- > tests/qapi-schema/double-type.json | 2 +- > tests/qapi-schema/double-type.out | 3 -- > tests/qapi-schema/enum-missing-data.err | 2 +- > tests/qapi-schema/indented-expr.json | 4 +- > tests/qapi-schema/indented-expr.out | 2 +- > tests/qapi-schema/missing-type.err | 1 + > tests/qapi-schema/missing-type.exit | 2 +- > tests/qapi-schema/missing-type.json | 2 +- > tests/qapi-schema/missing-type.out | 3 -- > tests/qapi-schema/unknown-expr-key.err | 1 + > tests/qapi-schema/unknown-expr-key.exit | 2 +- > tests/qapi-schema/unknown-expr-key.json | 2 +- > tests/qapi-schema/unknown-expr-key.out | 3 -- > 20 files changed, 75 insertions(+), 41 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 85aa8bf..8fbc45f 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -348,7 +348,29 @@ def check_exprs(schema): > if expr.has_key('enum'): > check_enum(expr, info) > > +def check_keys(expr_elem, meta, required, optional=[]): > + expr = expr_elem['expr'] > + info = expr_elem['info'] > + name = expr[meta] Caller ensures expr[meta] exists. Okay. > + if not isinstance(name, basestring): I'm a Python noob: why basestring and not str? > + raise QAPIExprError(info, > + "%s key must have a string value" % meta) > + expr_elem['name'] = name Where is this used? > + required.append(meta) Ugly side effect. To avoid, either make a new list here required = required + [ meta ] or do nothing here and... > + for (key, value) in expr.items(): > + if not key in required and not key in optional: ... add "and key != meta" to this condition. > + raise QAPIExprError(info, > + "%s '%s' has unknown key '%s'" > + % (meta, name, key)) > + for key in required: > + if not expr.has_key(key): > + raise QAPIExprError(info, > + "%s '%s' is missing key '%s'" > + % (meta, name, key)) > + > + > def parse_schema(input_file): > + # First pass: read entire file into memory > try: > schema = QAPISchema(open(input_file, "r")) > except (QAPISchemaError, QAPIExprError), e: > @@ -357,24 +379,44 @@ def parse_schema(input_file): > > exprs = [] > > - for expr_elem in schema.exprs: > - expr = expr_elem['expr'] > - if expr.has_key('enum'): > - add_enum(expr['enum'], expr.get('data')) > - elif expr.has_key('union'): > - add_union(expr) > - elif expr.has_key('type'): > - add_struct(expr) > - exprs.append(expr) > - > - # Try again for hidden UnionKind enum > - for expr_elem in schema.exprs: > - expr = expr_elem['expr'] > - if expr.has_key('union'): > - if not discriminator_find_enum_define(expr): > - add_enum('%sKind' % expr['union']) > - > try: > + # Next pass: learn the types and check for valid expression keys. At > + # this point, top-level 'include' has already been flattened. > + for expr_elem in schema.exprs: > + expr = expr_elem['expr'] > + if expr.has_key('enum'): > + check_keys(expr_elem, 'enum', ['data']) > + add_enum(expr['enum'], expr['data']) > + elif expr.has_key('union'): > + # Two styles of union, based on discriminator > + discriminator = expr.get('discriminator') > + if discriminator == {}: > + check_keys(expr_elem, 'union', ['data', 'discriminator']) > + else: > + check_keys(expr_elem, 'union', ['data'], > + ['base', 'discriminator']) > + add_union(expr) > + elif expr.has_key('type'): > + check_keys(expr_elem, 'type', ['data'], ['base']) > + add_struct(expr) > + elif expr.has_key('command'): > + check_keys(expr_elem, 'command', [], > + ['data', 'returns', 'gen', 'success-response']) > + elif expr.has_key('event'): > + check_keys(expr_elem, 'event', [], ['data']) > + else: > + raise QAPIExprError(expr_elem['info'], > + "expression is missing metatype") > + exprs.append(expr) > + > + # Try again for hidden UnionKind enum > + for expr_elem in schema.exprs: > + expr = expr_elem['expr'] > + if expr.has_key('union'): > + if not discriminator_find_enum_define(expr): > + add_enum('%sKind' % expr['union']) > + > + # Final pass - validate that exprs make sense > check_exprs(schema) > except QAPIExprError, e: > print >>sys.stderr, e This hunk is easier to review with whitespace ignored: @@ -356,13 +356,34 @@ exprs = [] - for expr_elem in schema.exprs: + try: + # Next pass: learn the types and check for valid expression keys. At + # this point, top-level 'include' has already been flattened. + for expr_elem in schema.exprs: expr = expr_elem['expr'] if expr.has_key('enum'): - add_enum(expr['enum'], expr.get('data')) + check_keys(expr_elem, 'enum', ['data']) + add_enum(expr['enum'], expr['data']) elif expr.has_key('union'): + # Two styles of union, based on discriminator + discriminator = expr.get('discriminator') + if discriminator == {}: + check_keys(expr_elem, 'union', ['data', 'discriminator']) + else: + check_keys(expr_elem, 'union', ['data'], + ['base', 'discriminator']) add_union(expr) elif expr.has_key('type'): + check_keys(expr_elem, 'type', ['data'], ['base']) add_struct(expr) + elif expr.has_key('command'): + check_keys(expr_elem, 'command', [], + ['data', 'returns', 'gen', 'success-response']) + elif expr.has_key('event'): + check_keys(expr_elem, 'event', [], ['data']) + else: + raise QAPIExprError(expr_elem['info'], + "expression is missing metatype") exprs.append(expr) # Try again for hidden UnionKind enum @@ -372,7 +393,7 @@ if not discriminator_find_enum_define(expr): add_enum('%sKind' % expr['union']) - try: + # Final pass - validate that exprs make sense check_exprs(schema) except QAPIExprError, e: print >>sys.stderr, e Looks good to me. The tests, too. [...]
On 09/23/2014 08:56 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> The previous commit demonstrated that the generator overlooked some >> fairly basic broken expressions: >> - missing metataype >> - metatype key has a non-string value >> - unknown key in relation to the metatype >> - conflicting metatype (this patch treats the second metatype as an >> unknown key of the first key visited, which is not necessarily the >> first key the user typed) > > The whole thing is a Saturday afternoon hack, with extra hacks bolted on > left & right. And this series is a sequence of MY Saturday afternoon hacks in cleaning it up :) > >> Add check_keys to cover these situations, and update testcases to >> match. A couple other tests (enum-missing-data, indented-expr) had >> to change since the validation added here occurs so early. >> >> 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> > >> +def check_keys(expr_elem, meta, required, optional=[]): >> + expr = expr_elem['expr'] >> + info = expr_elem['info'] >> + name = expr[meta] > > Caller ensures expr[meta] exists. Okay. > >> + if not isinstance(name, basestring): > > I'm a Python noob: why basestring and not str? Me too. No clue. Copy and paste from existing code. http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 > >> + raise QAPIExprError(info, >> + "%s key must have a string value" % meta) >> + expr_elem['name'] = name > > Where is this used? Hmm, I know I used it at one point in my series (to be able to print the name of an expression in check_type added in 13/19, without having to repeat the dance of if enum: expr['enum'] elif union: expr['union'] etc. in multiple places). Although in my refactoring, I may have eliminated the need for it after all. If it's not in use at the end of the series, I can drop it. > >> + required.append(meta) > > Ugly side effect. To avoid, either make a new list here > > required = required + [ meta ] > > or do nothing here and... > >> + for (key, value) in expr.items(): >> + if not key in required and not key in optional: > > ... add "and key != meta" to this condition. Shows my inexperience in python. Sure, I can fix. Looks like I get to spin a v5. > > This hunk is easier to review with whitespace ignored: Indeed. Alas, python is a stickler about whitespace reindentation. Since I have to respin, I'll probably split into two pieces (one with a no-op change that reindents, the other for the improvements).
Eric Blake <eblake@redhat.com> writes: > On 09/23/2014 08:56 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> The previous commit demonstrated that the generator overlooked some >>> fairly basic broken expressions: >>> - missing metataype >>> - metatype key has a non-string value >>> - unknown key in relation to the metatype >>> - conflicting metatype (this patch treats the second metatype as an >>> unknown key of the first key visited, which is not necessarily the >>> first key the user typed) >> >> The whole thing is a Saturday afternoon hack, with extra hacks bolted on >> left & right. > > And this series is a sequence of MY Saturday afternoon hacks in cleaning > it up :) Much appreciated! >>> Add check_keys to cover these situations, and update testcases to >>> match. A couple other tests (enum-missing-data, indented-expr) had >>> to change since the validation added here occurs so early. >>> >>> 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> >> > >>> +def check_keys(expr_elem, meta, required, optional=[]): >>> + expr = expr_elem['expr'] >>> + info = expr_elem['info'] >>> + name = expr[meta] >> >> Caller ensures expr[meta] exists. Okay. >> >>> + if not isinstance(name, basestring): >> >> I'm a Python noob: why basestring and not str? > > Me too. No clue. Copy and paste from existing code. > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 Yes. It's our only use of basestring. Other places use isinstance(FOO, str). The basestring use comes from Kevin's commit b35284e. Kevin, why basestring and not str? >> >>> + raise QAPIExprError(info, >>> + "%s key must have a string value" % meta) >>> + expr_elem['name'] = name >> >> Where is this used? > > Hmm, I know I used it at one point in my series (to be able to print the > name of an expression in check_type added in 13/19, without having to > repeat the dance of if enum: expr['enum'] elif union: expr['union'] etc. > in multiple places). Although in my refactoring, I may have eliminated > the need for it after all. If it's not in use at the end of the series, > I can drop it. A quick grep comes up empty. >>> + required.append(meta) >> >> Ugly side effect. To avoid, either make a new list here >> >> required = required + [ meta ] >> >> or do nothing here and... >> >>> + for (key, value) in expr.items(): >>> + if not key in required and not key in optional: >> >> ... add "and key != meta" to this condition. > > Shows my inexperience in python. Sure, I can fix. Looks like I get to > spin a v5. > >> >> This hunk is easier to review with whitespace ignored: > > Indeed. Alas, python is a stickler about whitespace reindentation. > Since I have to respin, I'll probably split into two pieces (one with a > no-op change that reindents, the other for the improvements). Not necessary for me, I'm comfy with telling Emacs to hide whitespace differences :)
Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben: > Eric Blake <eblake@redhat.com> writes: > > > On 09/23/2014 08:56 AM, Markus Armbruster wrote: > >> Eric Blake <eblake@redhat.com> writes: > >>> Add check_keys to cover these situations, and update testcases to > >>> match. A couple other tests (enum-missing-data, indented-expr) had > >>> to change since the validation added here occurs so early. > >>> > >>> 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> > >> > > > >>> +def check_keys(expr_elem, meta, required, optional=[]): > >>> + expr = expr_elem['expr'] > >>> + info = expr_elem['info'] > >>> + name = expr[meta] > >> > >> Caller ensures expr[meta] exists. Okay. > >> > >>> + if not isinstance(name, basestring): > >> > >> I'm a Python noob: why basestring and not str? > > > > Me too. No clue. Copy and paste from existing code. > > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 > > Yes. It's our only use of basestring. Other places use isinstance(FOO, > str). > > The basestring use comes from Kevin's commit b35284e. Kevin, why > basestring and not str? Do I look as if I knew what I'm doing when I write Python code? :-) Apparently basestring is the superclass for ASCII and Unicode strings. I seem to dimly remember that I did indeed get a Unicode string somewhere (even though probably no non-ASCII characters in it) and it caused trouble. Might well have been here. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben: >> Eric Blake <eblake@redhat.com> writes: >> >> > On 09/23/2014 08:56 AM, Markus Armbruster wrote: >> >> Eric Blake <eblake@redhat.com> writes: >> >>> Add check_keys to cover these situations, and update testcases to >> >>> match. A couple other tests (enum-missing-data, indented-expr) had >> >>> to change since the validation added here occurs so early. >> >>> >> >>> 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> >> >> >> > >> >>> +def check_keys(expr_elem, meta, required, optional=[]): >> >>> + expr = expr_elem['expr'] >> >>> + info = expr_elem['info'] >> >>> + name = expr[meta] >> >> >> >> Caller ensures expr[meta] exists. Okay. >> >> >> >>> + if not isinstance(name, basestring): >> >> >> >> I'm a Python noob: why basestring and not str? >> > >> > Me too. No clue. Copy and paste from existing code. >> > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 >> >> Yes. It's our only use of basestring. Other places use isinstance(FOO, >> str). >> >> The basestring use comes from Kevin's commit b35284e. Kevin, why >> basestring and not str? > > Do I look as if I knew what I'm doing when I write Python code? :-) > > Apparently basestring is the superclass for ASCII and Unicode strings. I > seem to dimly remember that I did indeed get a Unicode string somewhere > (even though probably no non-ASCII characters in it) and it caused > trouble. Might well have been here. Aha. These links apply: https://stackoverflow.com/questions/11301138/how-to-check-if-variable-is-string-with-python-2-and-3-compatibility https://en.wikipedia.org/wiki/File:Pieter_Bruegel_the_Elder_%281568%29_The_Blind_Leading_the_Blind.jpg
Kevin Wolf <kwolf@redhat.com> writes: > Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben: >> Eric Blake <eblake@redhat.com> writes: >> >> > On 09/23/2014 08:56 AM, Markus Armbruster wrote: >> >> Eric Blake <eblake@redhat.com> writes: >> >>> Add check_keys to cover these situations, and update testcases to >> >>> match. A couple other tests (enum-missing-data, indented-expr) had >> >>> to change since the validation added here occurs so early. >> >>> >> >>> 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> >> >> >> > >> >>> +def check_keys(expr_elem, meta, required, optional=[]): >> >>> + expr = expr_elem['expr'] >> >>> + info = expr_elem['info'] >> >>> + name = expr[meta] >> >> >> >> Caller ensures expr[meta] exists. Okay. >> >> >> >>> + if not isinstance(name, basestring): >> >> >> >> I'm a Python noob: why basestring and not str? >> > >> > Me too. No clue. Copy and paste from existing code. >> > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 >> >> Yes. It's our only use of basestring. Other places use isinstance(FOO, >> str). >> >> The basestring use comes from Kevin's commit b35284e. Kevin, why >> basestring and not str? > > Do I look as if I knew what I'm doing when I write Python code? :-) > > Apparently basestring is the superclass for ASCII and Unicode strings. I > seem to dimly remember that I did indeed get a Unicode string somewhere > (even though probably no non-ASCII characters in it) and it caused > trouble. Might well have been here. I think there are two sane ways forward: 1. Declare QAPI schemas to be ASCII only for now. Replace the one instance of basestring by plain str. 2. Fix the code to work with both ASCII and Unicode strings even in Python 2. Package six could be used. I'm voting for 1.
Am 26.09.2014 um 11:15 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben: > >> Eric Blake <eblake@redhat.com> writes: > >> > >> > On 09/23/2014 08:56 AM, Markus Armbruster wrote: > >> >> Eric Blake <eblake@redhat.com> writes: > >> >>> Add check_keys to cover these situations, and update testcases to > >> >>> match. A couple other tests (enum-missing-data, indented-expr) had > >> >>> to change since the validation added here occurs so early. > >> >>> > >> >>> 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> > >> >> > >> > > >> >>> +def check_keys(expr_elem, meta, required, optional=[]): > >> >>> + expr = expr_elem['expr'] > >> >>> + info = expr_elem['info'] > >> >>> + name = expr[meta] > >> >> > >> >> Caller ensures expr[meta] exists. Okay. > >> >> > >> >>> + if not isinstance(name, basestring): > >> >> > >> >> I'm a Python noob: why basestring and not str? > >> > > >> > Me too. No clue. Copy and paste from existing code. > >> > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 > >> > >> Yes. It's our only use of basestring. Other places use isinstance(FOO, > >> str). > >> > >> The basestring use comes from Kevin's commit b35284e. Kevin, why > >> basestring and not str? > > > > Do I look as if I knew what I'm doing when I write Python code? :-) > > > > Apparently basestring is the superclass for ASCII and Unicode strings. I > > seem to dimly remember that I did indeed get a Unicode string somewhere > > (even though probably no non-ASCII characters in it) and it caused > > trouble. Might well have been here. > > I think there are two sane ways forward: > > 1. Declare QAPI schemas to be ASCII only for now. Replace the one > instance of basestring by plain str. > > 2. Fix the code to work with both ASCII and Unicode strings even in > Python 2. Package six could be used. > > I'm voting for 1. As I said, the problem is probably not about the actual value of the string (I doubt we use anything but ASCII at the moment), but about its type. We may need to convert something somewhere. If you think it's worth the effort of finding that place, go ahead. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 26.09.2014 um 11:15 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben: >> >> Eric Blake <eblake@redhat.com> writes: >> >> >> >> > On 09/23/2014 08:56 AM, Markus Armbruster wrote: >> >> >> Eric Blake <eblake@redhat.com> writes: >> >> >>> Add check_keys to cover these situations, and update testcases to >> >> >>> match. A couple other tests (enum-missing-data, indented-expr) had >> >> >>> to change since the validation added here occurs so early. >> >> >>> >> >> >>> 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> >> >> >> >> >> > >> >> >>> +def check_keys(expr_elem, meta, required, optional=[]): >> >> >>> + expr = expr_elem['expr'] >> >> >>> + info = expr_elem['info'] >> >> >>> + name = expr[meta] >> >> >> >> >> >> Caller ensures expr[meta] exists. Okay. >> >> >> >> >> >>> + if not isinstance(name, basestring): >> >> >> >> >> >> I'm a Python noob: why basestring and not str? >> >> > >> >> > Me too. No clue. Copy and paste from existing code. >> >> > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 >> >> >> >> Yes. It's our only use of basestring. Other places use isinstance(FOO, >> >> str). >> >> >> >> The basestring use comes from Kevin's commit b35284e. Kevin, why >> >> basestring and not str? >> > >> > Do I look as if I knew what I'm doing when I write Python code? :-) >> > >> > Apparently basestring is the superclass for ASCII and Unicode strings. I >> > seem to dimly remember that I did indeed get a Unicode string somewhere >> > (even though probably no non-ASCII characters in it) and it caused >> > trouble. Might well have been here. >> >> I think there are two sane ways forward: >> >> 1. Declare QAPI schemas to be ASCII only for now. Replace the one >> instance of basestring by plain str. >> >> 2. Fix the code to work with both ASCII and Unicode strings even in >> Python 2. Package six could be used. >> >> I'm voting for 1. > > As I said, the problem is probably not about the actual value of the > string (I doubt we use anything but ASCII at the moment), but about its > type. We may need to convert something somewhere. If you think it's > worth the effort of finding that place, go ahead. A value always has a concrete type. I'd expect all string values in the qapi scripts to be of type str. We have to mess with Python 2's basestring only where we need to cope with string values that may be either of type str or of type unicode. Since there should not be any values of type unicode, I don't see why we'd want to do the extra work required for 2.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 85aa8bf..8fbc45f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -348,7 +348,29 @@ def check_exprs(schema): if expr.has_key('enum'): check_enum(expr, info) +def check_keys(expr_elem, meta, required, optional=[]): + expr = expr_elem['expr'] + info = expr_elem['info'] + name = expr[meta] + if not isinstance(name, basestring): + raise QAPIExprError(info, + "%s key must have a string value" % meta) + expr_elem['name'] = name + required.append(meta) + for (key, value) in expr.items(): + if not key in required and not key in optional: + raise QAPIExprError(info, + "%s '%s' has unknown key '%s'" + % (meta, name, key)) + for key in required: + if not expr.has_key(key): + raise QAPIExprError(info, + "%s '%s' is missing key '%s'" + % (meta, name, key)) + + def parse_schema(input_file): + # First pass: read entire file into memory try: schema = QAPISchema(open(input_file, "r")) except (QAPISchemaError, QAPIExprError), e: @@ -357,24 +379,44 @@ def parse_schema(input_file): exprs = [] - for expr_elem in schema.exprs: - expr = expr_elem['expr'] - if expr.has_key('enum'): - add_enum(expr['enum'], expr.get('data')) - elif expr.has_key('union'): - add_union(expr) - elif expr.has_key('type'): - add_struct(expr) - exprs.append(expr) - - # Try again for hidden UnionKind enum - for expr_elem in schema.exprs: - expr = expr_elem['expr'] - if expr.has_key('union'): - if not discriminator_find_enum_define(expr): - add_enum('%sKind' % expr['union']) - try: + # Next pass: learn the types and check for valid expression keys. At + # this point, top-level 'include' has already been flattened. + for expr_elem in schema.exprs: + expr = expr_elem['expr'] + if expr.has_key('enum'): + check_keys(expr_elem, 'enum', ['data']) + add_enum(expr['enum'], expr['data']) + elif expr.has_key('union'): + # Two styles of union, based on discriminator + discriminator = expr.get('discriminator') + if discriminator == {}: + check_keys(expr_elem, 'union', ['data', 'discriminator']) + else: + check_keys(expr_elem, 'union', ['data'], + ['base', 'discriminator']) + add_union(expr) + elif expr.has_key('type'): + check_keys(expr_elem, 'type', ['data'], ['base']) + add_struct(expr) + elif expr.has_key('command'): + check_keys(expr_elem, 'command', [], + ['data', 'returns', 'gen', 'success-response']) + elif expr.has_key('event'): + check_keys(expr_elem, 'event', [], ['data']) + else: + raise QAPIExprError(expr_elem['info'], + "expression is missing metatype") + exprs.append(expr) + + # Try again for hidden UnionKind enum + for expr_elem in schema.exprs: + expr = expr_elem['expr'] + if expr.has_key('union'): + if not discriminator_find_enum_define(expr): + add_enum('%sKind' % expr['union']) + + # Final pass - validate that exprs make sense check_exprs(schema) except QAPIExprError, e: print >>sys.stderr, e diff --git a/tests/qapi-schema/bad-type-dict.err b/tests/qapi-schema/bad-type-dict.err index e69de29..b7c73cc 100644 --- a/tests/qapi-schema/bad-type-dict.err +++ b/tests/qapi-schema/bad-type-dict.err @@ -0,0 +1 @@ +tests/qapi-schema/bad-type-dict.json:2: command key must have a string value diff --git a/tests/qapi-schema/bad-type-dict.exit b/tests/qapi-schema/bad-type-dict.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/bad-type-dict.exit +++ b/tests/qapi-schema/bad-type-dict.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/bad-type-dict.json b/tests/qapi-schema/bad-type-dict.json index 3c392a7..2a91b24 100644 --- a/tests/qapi-schema/bad-type-dict.json +++ b/tests/qapi-schema/bad-type-dict.json @@ -1,2 +1,2 @@ -# FIXME: we should reject an expression with a metatype that is not a string +# we reject an expression with a metatype that is not a string { 'command': { } } diff --git a/tests/qapi-schema/bad-type-dict.out b/tests/qapi-schema/bad-type-dict.out index c62f1ed..e69de29 100644 --- a/tests/qapi-schema/bad-type-dict.out +++ b/tests/qapi-schema/bad-type-dict.out @@ -1,3 +0,0 @@ -[OrderedDict([('command', OrderedDict())])] -[] -[] diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err index e69de29..394127b 100644 --- a/tests/qapi-schema/double-type.err +++ b/tests/qapi-schema/double-type.err @@ -0,0 +1 @@ +tests/qapi-schema/double-type.json:2: type 'bar' has unknown key 'command' diff --git a/tests/qapi-schema/double-type.exit b/tests/qapi-schema/double-type.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/double-type.exit +++ b/tests/qapi-schema/double-type.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/double-type.json b/tests/qapi-schema/double-type.json index 6ca96b9..471623a 100644 --- a/tests/qapi-schema/double-type.json +++ b/tests/qapi-schema/double-type.json @@ -1,2 +1,2 @@ -# FIXME: we should reject an expression with ambiguous metatype +# we reject an expression with ambiguous metatype { 'command': 'foo', 'type': 'bar', 'data': { } } diff --git a/tests/qapi-schema/double-type.out b/tests/qapi-schema/double-type.out index 3e244f5..e69de29 100644 --- a/tests/qapi-schema/double-type.out +++ b/tests/qapi-schema/double-type.out @@ -1,3 +0,0 @@ -[OrderedDict([('command', 'foo'), ('type', 'bar'), ('data', OrderedDict())])] -[] -[OrderedDict([('command', 'foo'), ('type', 'bar'), ('data', OrderedDict())])] diff --git a/tests/qapi-schema/enum-missing-data.err b/tests/qapi-schema/enum-missing-data.err index 4b8b59e..62f4a7f 100644 --- a/tests/qapi-schema/enum-missing-data.err +++ b/tests/qapi-schema/enum-missing-data.err @@ -1 +1 @@ -tests/qapi-schema/enum-missing-data.json:2: enum 'MyEnum' requires an array for 'data' +tests/qapi-schema/enum-missing-data.json:2: enum 'MyEnum' is missing key 'data' diff --git a/tests/qapi-schema/indented-expr.json b/tests/qapi-schema/indented-expr.json index d80af60..7115d31 100644 --- a/tests/qapi-schema/indented-expr.json +++ b/tests/qapi-schema/indented-expr.json @@ -1,2 +1,2 @@ -{ 'id' : 'eins' } - { 'id' : 'zwei' } +{ 'command' : 'eins' } + { 'command' : 'zwei' } diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out index 98af89a..b5ce915 100644 --- a/tests/qapi-schema/indented-expr.out +++ b/tests/qapi-schema/indented-expr.out @@ -1,3 +1,3 @@ -[OrderedDict([('id', 'eins')]), OrderedDict([('id', 'zwei')])] +[OrderedDict([('command', 'eins')]), OrderedDict([('command', 'zwei')])] [] [] diff --git a/tests/qapi-schema/missing-type.err b/tests/qapi-schema/missing-type.err index e69de29..19b7c49 100644 --- a/tests/qapi-schema/missing-type.err +++ b/tests/qapi-schema/missing-type.err @@ -0,0 +1 @@ +tests/qapi-schema/missing-type.json:2: expression is missing metatype diff --git a/tests/qapi-schema/missing-type.exit b/tests/qapi-schema/missing-type.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/missing-type.exit +++ b/tests/qapi-schema/missing-type.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/missing-type.json b/tests/qapi-schema/missing-type.json index 1696f5c..ff5349d 100644 --- a/tests/qapi-schema/missing-type.json +++ b/tests/qapi-schema/missing-type.json @@ -1,2 +1,2 @@ -# FIXME: we should reject an expression with missing metatype +# we reject an expression with missing metatype { 'data': { } } diff --git a/tests/qapi-schema/missing-type.out b/tests/qapi-schema/missing-type.out index 67fd4fa..e69de29 100644 --- a/tests/qapi-schema/missing-type.out +++ b/tests/qapi-schema/missing-type.out @@ -1,3 +0,0 @@ -[OrderedDict([('data', OrderedDict())])] -[] -[] diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err index e69de29..3b35508 100644 --- a/tests/qapi-schema/unknown-expr-key.err +++ b/tests/qapi-schema/unknown-expr-key.err @@ -0,0 +1 @@ +tests/qapi-schema/unknown-expr-key.json:2: type 'bar' has unknown key 'bogus' diff --git a/tests/qapi-schema/unknown-expr-key.exit b/tests/qapi-schema/unknown-expr-key.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/unknown-expr-key.exit +++ b/tests/qapi-schema/unknown-expr-key.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/unknown-expr-key.json b/tests/qapi-schema/unknown-expr-key.json index 1e9282d..ba7bdf3 100644 --- a/tests/qapi-schema/unknown-expr-key.json +++ b/tests/qapi-schema/unknown-expr-key.json @@ -1,2 +1,2 @@ -# FIXME: we should reject an expression with unknown top-level keys +# we reject an expression with unknown top-level keys { 'type': 'bar', 'data': { 'string': 'str'}, 'bogus': { } } diff --git a/tests/qapi-schema/unknown-expr-key.out b/tests/qapi-schema/unknown-expr-key.out index c93f020..e69de29 100644 --- a/tests/qapi-schema/unknown-expr-key.out +++ b/tests/qapi-schema/unknown-expr-key.out @@ -1,3 +0,0 @@ -[OrderedDict([('type', 'bar'), ('data', OrderedDict([('string', 'str')])), ('bogus', OrderedDict())])] -[] -[OrderedDict([('type', 'bar'), ('data', OrderedDict([('string', 'str')])), ('bogus', OrderedDict())])]
The previous commit demonstrated that the generator overlooked some fairly basic broken expressions: - missing metataype - metatype key has a non-string value - unknown key in relation to the metatype - conflicting metatype (this patch treats the second metatype as an unknown key of the first key visited, which is not necessarily the first key the user typed) Add check_keys to cover these situations, and update testcases to match. A couple other tests (enum-missing-data, indented-expr) had to change since the validation added here occurs so early. 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 | 76 +++++++++++++++++++++++++-------- tests/qapi-schema/bad-type-dict.err | 1 + tests/qapi-schema/bad-type-dict.exit | 2 +- tests/qapi-schema/bad-type-dict.json | 2 +- tests/qapi-schema/bad-type-dict.out | 3 -- tests/qapi-schema/double-type.err | 1 + tests/qapi-schema/double-type.exit | 2 +- tests/qapi-schema/double-type.json | 2 +- tests/qapi-schema/double-type.out | 3 -- tests/qapi-schema/enum-missing-data.err | 2 +- tests/qapi-schema/indented-expr.json | 4 +- tests/qapi-schema/indented-expr.out | 2 +- tests/qapi-schema/missing-type.err | 1 + tests/qapi-schema/missing-type.exit | 2 +- tests/qapi-schema/missing-type.json | 2 +- tests/qapi-schema/missing-type.out | 3 -- tests/qapi-schema/unknown-expr-key.err | 1 + tests/qapi-schema/unknown-expr-key.exit | 2 +- tests/qapi-schema/unknown-expr-key.json | 2 +- tests/qapi-schema/unknown-expr-key.out | 3 -- 20 files changed, 75 insertions(+), 41 deletions(-)