| Message ID | 1393499376-4374-5-git-send-email-wenchaoqemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 02/27/2014 04:09 AM, Wenchao Xia wrote: > From: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com> Double-S-o-B. I've also noticed that I'm getting undeliverable mail rejections from your linux.vnet.ibm.com address: TCVM.MEGACENTER.DE.IBM.COM unable to deliver following mail to recipient(s): <xiawenc@linux.ibm.com> TCVM.MEGACENTER.DE.IBM.COM received negative reply: 550 5.1.1 <xiawenc@linux.ibm.com>: Recipient address rejected: User unknown in local recipient table > --- > scripts/qapi.py | 106 +++++++++++++++++++- > tests/Makefile | 4 +- > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 1954292..cea346f 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -50,6 +50,15 @@ class QAPISchemaError(Exception): > def __str__(self): > return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg) > > +class QAPIExprError(Exception): > + def __init__(self, expr_info, msg): > + self.fp = expr_info['fp'] > + self.line = expr_info['line'] > + self.msg = msg > + > + def __str__(self): > + return "%s:%s: %s" % (self.fp.name, self.line, self.msg) > + > class QAPISchema: > > def __init__(self, fp): > @@ -64,7 +73,10 @@ class QAPISchema: > self.accept() > > while self.tok != None: > - self.exprs.append(self.get_expr(False)) > + expr_info = {'fp': fp, 'line': self.line} > + expr_elem = {'expr': self.get_expr(False), > + 'info': expr_info} > + self.exprs.append(expr_elem) Should these two hunks be part of 3/10? Or at least as a separate patch? Or at least mentioned in the commit message? > @@ -162,6 +174,89 @@ class QAPISchema: > raise QAPISchemaError(self, 'Expected "{", "[" or string') > return expr > > +def find_base_fields(base): > + base_struct_define = find_struct(base) > + if not base_struct_define: > + return None > + return base_struct_define['data'] > + > +# Return the discriminator enum define if discrminator is specified as an s/discrminator/discriminator/
于 2014/2/28 3:21, Eric Blake 写道: > On 02/27/2014 04:09 AM, Wenchao Xia wrote: >> From: Wenchao Xia<xiawenc@linux.vnet.ibm.com> >> >> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com> >> Signed-off-by: Wenchao Xia<wenchaoqemu@gmail.com> > Double-S-o-B. I've also noticed that I'm getting undeliverable mail > rejections from your linux.vnet.ibm.com address: > > TCVM.MEGACENTER.DE.IBM.COM unable to deliver following mail to recipient(s): > <xiawenc@linux.ibm.com> > TCVM.MEGACENTER.DE.IBM.COM received negative reply: > 550 5.1.1<xiawenc@linux.ibm.com>: Recipient address rejected: User > unknown in local recipient table > I am using wenchaoqemu@gmail.com now, yep I should fix the SoB problem. >> --- >> scripts/qapi.py | 106 +++++++++++++++++++- >> tests/Makefile | 4 +- >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index 1954292..cea346f 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception): >> def __str__(self): >> return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg) >> >> +class QAPIExprError(Exception): >> + def __init__(self, expr_info, msg): >> + self.fp = expr_info['fp'] >> + self.line = expr_info['line'] >> + self.msg = msg >> + >> + def __str__(self): >> + return "%s:%s: %s" % (self.fp.name, self.line, self.msg) >> + >> class QAPISchema: >> >> def __init__(self, fp): >> @@ -64,7 +73,10 @@ class QAPISchema: >> self.accept() >> >> while self.tok != None: >> - self.exprs.append(self.get_expr(False)) >> + expr_info = {'fp': fp, 'line': self.line} >> + expr_elem = {'expr': self.get_expr(False), >> + 'info': expr_info} >> + self.exprs.append(expr_elem) > Should these two hunks be part of 3/10? Or at least as a separate > patch? Or at least mentioned in the commit message? > Patch 3/10 only remember line number in class QAPISchema, as well as its member "cursor". The above code add line info in exprs, and if they are moved to last patch, the caller code should also go, which is a main part of patch. I'd like to improve the commit messages instead. >> @@ -162,6 +174,89 @@ class QAPISchema: >> raise QAPISchemaError(self, 'Expected "{", "[" or string') >> return expr >> >> +def find_base_fields(base): >> + base_struct_define = find_struct(base) >> + if not base_struct_define: >> + return None >> + return base_struct_define['data'] >> + >> +# Return the discriminator enum define if discrminator is specified as an > s/discrminator/discriminator/ > >
Wenchao Xia <wenchaoqemu@gmail.com> writes: > From: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Commit message lost detail since v7. Intentional? > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com> > --- > scripts/qapi.py | 106 +++++++++++++++++++- > tests/Makefile | 4 +- > .../qapi-schema/flat-union-invalid-branch-key.err | 1 + > .../qapi-schema/flat-union-invalid-branch-key.exit | 1 + > .../qapi-schema/flat-union-invalid-branch-key.json | 17 +++ > .../flat-union-invalid-discriminator.err | 1 + > .../flat-union-invalid-discriminator.exit | 1 + > .../flat-union-invalid-discriminator.json | 17 +++ > tests/qapi-schema/flat-union-no-base.err | 1 + > tests/qapi-schema/flat-union-no-base.exit | 1 + > tests/qapi-schema/flat-union-no-base.json | 16 +++ > tests/qapi-schema/union-invalid-base.err | 1 + > tests/qapi-schema/union-invalid-base.exit | 1 + > tests/qapi-schema/union-invalid-base.json | 10 ++ > 14 files changed, 175 insertions(+), 3 deletions(-) > create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err > create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit > create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json > create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out > create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err > create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit > create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json > create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out > create mode 100644 tests/qapi-schema/flat-union-no-base.err > create mode 100644 tests/qapi-schema/flat-union-no-base.exit > create mode 100644 tests/qapi-schema/flat-union-no-base.json > create mode 100644 tests/qapi-schema/flat-union-no-base.out > create mode 100644 tests/qapi-schema/union-invalid-base.err > create mode 100644 tests/qapi-schema/union-invalid-base.exit > create mode 100644 tests/qapi-schema/union-invalid-base.json > create mode 100644 tests/qapi-schema/union-invalid-base.out > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 1954292..cea346f 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -50,6 +50,15 @@ class QAPISchemaError(Exception): > def __str__(self): > return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg) > > +class QAPIExprError(Exception): > + def __init__(self, expr_info, msg): > + self.fp = expr_info['fp'] > + self.line = expr_info['line'] > + self.msg = msg > + > + def __str__(self): > + return "%s:%s: %s" % (self.fp.name, self.line, self.msg) > + > class QAPISchema: > > def __init__(self, fp): > @@ -64,7 +73,10 @@ class QAPISchema: > self.accept() > > while self.tok != None: > - self.exprs.append(self.get_expr(False)) > + expr_info = {'fp': fp, 'line': self.line} > + expr_elem = {'expr': self.get_expr(False), > + 'info': expr_info} > + self.exprs.append(expr_elem) > > def accept(self): > while True: > @@ -162,6 +174,89 @@ class QAPISchema: > raise QAPISchemaError(self, 'Expected "{", "[" or string') > return expr > > +def find_base_fields(base): > + base_struct_define = find_struct(base) > + if not base_struct_define: > + return None > + return base_struct_define['data'] > + > +# Return the discriminator enum define if discrminator is specified as an > +# enum type, otherwise return None. > +def discriminator_find_enum_define(expr): > + base = expr.get('base') > + discriminator = expr.get('discriminator') Why not expr['base'] and expr['discriminator']? More of the same below. No need to respin just for that; we can clean it up on top. > + > + if not (discriminator and base): > + return None > + > + base_fields = find_base_fields(base) > + if not base_fields: > + return None > + > + discriminator_type = base_fields.get(discriminator) > + if not discriminator_type: > + return None > + > + return find_enum(discriminator_type) > + > +def check_union(expr, expr_info): > + name = expr['union'] > + base = expr.get('base') > + discriminator = expr.get('discriminator') > + members = expr['data'] > + > + # If the object has a member 'base', its value must name a complex type. > + if base: > + base_fields = find_base_fields(base) > + if not base_fields: > + raise QAPIExprError(expr_info, > + "Base '%s' is not a valid type" > + % base) > + > + # If the union object has no member 'discriminator', it's an > + # ordinary union. > + if not discriminator: > + enum_define = None > + > + # Else if the value of member 'discriminator' is {}, it's an > + # anonymous union. > + elif discriminator == {}: > + enum_define = None > + > + # Else, it's a flat union. > + else: > + # The object must have a member 'base'. > + if not base: > + raise QAPIExprError(expr_info, > + "Flat union '%s' must have a base field" > + % name) > + # The value of member 'discriminator' must name a member of the > + # base type. > + if not base_fields.get(discriminator): > + raise QAPIExprError(expr_info, > + "Discriminator '%s' is not a member of base " > + "type '%s'" > + % (discriminator, base)) > + enum_define = discriminator_find_enum_define(expr) Could this be simplified? Like this: # The value of member 'discriminator' must name a member of the # base type. discriminator_type = base_fields.get(discriminator): if not discriminator_type raise QAPIExprError(expr_info, "Discriminator '%s' is not a member of base " "type '%s'" % (discriminator, base)) enum_define = find_enum(discriminator_type) It's the only use of discriminator_find_enum_define()... Hmm, later patches add more uses, so my simplification is probably not worthwhile. Anyway, we can simplify on top. > + > + # Check every branch > + for (key, value) in members.items(): > + # If this named member's value names an enum type, then all members > + # of 'data' must also be members of the enum type. > + if enum_define and not key in enum_define['enum_values']: > + raise QAPIExprError(expr_info, > + "Discriminator value '%s' is not found in " > + "enum '%s'" % > + (key, enum_define["enum_name"])) > + # Todo: put more check such as whether each value is valid, but it may > + # need more functions to handle array case, so leave it now. I'm not sure I get your comment. > + > +def check_exprs(schema): > + for expr_elem in schema.exprs: > + expr = expr_elem['expr'] > + if expr.has_key('union'): > + check_union(expr, expr_elem['info']) > + > def parse_schema(fp): > try: > schema = QAPISchema(fp) > @@ -171,7 +266,8 @@ def parse_schema(fp): > > exprs = [] > > - for expr in schema.exprs: > + for expr_elem in schema.exprs: > + expr = expr_elem['expr'] > if expr.has_key('enum'): > add_enum(expr['enum'], expr['data']) > elif expr.has_key('union'): > @@ -181,6 +277,12 @@ def parse_schema(fp): > add_struct(expr) > exprs.append(expr) > > + try: > + check_exprs(schema) > + except QAPIExprError, e: > + print >>sys.stderr, e > + exit(1) > + > return exprs > > def parse_args(typeinfo): > diff --git a/tests/Makefile b/tests/Makefile > index dfe06eb..6ac9889 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ > qapi-schema-test.json quoted-structural-chars.json \ > trailing-comma-list.json trailing-comma-object.json \ > unclosed-list.json unclosed-object.json unclosed-string.json \ > - duplicate-key.json) > + duplicate-key.json union-invalid-base.json flat-union-no-base.json \ > + flat-union-invalid-discriminator.json \ > + flat-union-invalid-branch-key.json) > > GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h > > diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err > new file mode 100644 > index 0000000..1125caf > --- /dev/null > +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err > @@ -0,0 +1 @@ > +<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum' > diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json > new file mode 100644 > index 0000000..a624282 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-invalid-branch-key.json > @@ -0,0 +1,17 @@ > +{ 'enum': 'TestEnum', > + 'data': [ 'value1', 'value2' ] } > + > +{ 'type': 'TestBase', > + 'data': { 'enum1': 'TestEnum' } } > + > +{ 'type': 'TestTypeA', > + 'data': { 'string': 'str' } } > + > +{ 'type': 'TestTypeB', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'base': 'TestBase', > + 'discriminator': 'enum1', > + 'data': { 'value_wrong': 'TestTypeA', > + 'value2': 'TestTypeB' } } > diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err > new file mode 100644 > index 0000000..cad9dbf > --- /dev/null > +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err > @@ -0,0 +1 @@ > +<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase' > diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json > new file mode 100644 > index 0000000..887157e > --- /dev/null > +++ b/tests/qapi-schema/flat-union-invalid-discriminator.json > @@ -0,0 +1,17 @@ > +{ 'enum': 'TestEnum', > + 'data': [ 'value1', 'value2' ] } > + > +{ 'type': 'TestBase', > + 'data': { 'enum1': 'TestEnum' } } > + > +{ 'type': 'TestTypeA', > + 'data': { 'string': 'str' } } > + > +{ 'type': 'TestTypeB', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'base': 'TestBase', > + 'discriminator': 'enum_wrong', > + 'data': { 'value1': 'TestTypeA', > + 'value2': 'TestTypeB' } } > diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err > new file mode 100644 > index 0000000..e695315 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-no-base.err > @@ -0,0 +1 @@ > +<stdin>:13: Flat union 'TestUnion' must have a base field > diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-no-base.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json > new file mode 100644 > index 0000000..e0900d4 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-no-base.json > @@ -0,0 +1,16 @@ > +{ 'enum': 'TestEnum', > + 'data': [ 'value1', 'value2' ] } > + > +{ 'type': 'TestBase', > + 'data': { 'enum1': 'TestEnum' } } > + TestEnum and TestBase aren't used. > +{ 'type': 'TestTypeA', > + 'data': { 'string': 'str' } } > + > +{ 'type': 'TestTypeB', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'discriminator': 'enum1', > + 'data': { 'value1': 'TestTypeA', > + 'value2': 'TestTypeB' } } > diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err > new file mode 100644 > index 0000000..dd8e3d1 > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-base.err > @@ -0,0 +1 @@ > +<stdin>:7: Base 'TestBaseWrong' is not a valid type > diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-base.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json > new file mode 100644 > index 0000000..1fa4930 > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-base.json > @@ -0,0 +1,10 @@ > +{ 'type': 'TestTypeA', > + 'data': { 'string': 'str' } } > + > +{ 'type': 'TestTypeB', > + 'data': { 'integer': 'int' } } > + > +{ 'union': 'TestUnion', > + 'base': 'TestBaseWrong', > + 'data': { 'value1': 'TestTypeA', > + 'value2': 'TestTypeB' } } > diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out > new file mode 100644 > index 0000000..e69de29 Tests cover all the new errors. Good.
于 2014/3/4 20:47, Markus Armbruster 写道: > Wenchao Xia <wenchaoqemu@gmail.com> writes: > >> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > Commit message lost detail since v7. Intentional? > I thought you don't want the message in V7, maybe I misunderstood it. >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com> >> --- >> scripts/qapi.py | 106 +++++++++++++++++++- >> tests/Makefile | 4 +- >> .../qapi-schema/flat-union-invalid-branch-key.err | 1 + >> .../qapi-schema/flat-union-invalid-branch-key.exit | 1 + >> .../qapi-schema/flat-union-invalid-branch-key.json | 17 +++ >> .../flat-union-invalid-discriminator.err | 1 + >> .../flat-union-invalid-discriminator.exit | 1 + >> .../flat-union-invalid-discriminator.json | 17 +++ >> tests/qapi-schema/flat-union-no-base.err | 1 + >> tests/qapi-schema/flat-union-no-base.exit | 1 + >> tests/qapi-schema/flat-union-no-base.json | 16 +++ >> tests/qapi-schema/union-invalid-base.err | 1 + >> tests/qapi-schema/union-invalid-base.exit | 1 + >> tests/qapi-schema/union-invalid-base.json | 10 ++ >> 14 files changed, 175 insertions(+), 3 deletions(-) >> create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err >> create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit >> create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json >> create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out >> create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err >> create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit >> create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json >> create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out >> create mode 100644 tests/qapi-schema/flat-union-no-base.err >> create mode 100644 tests/qapi-schema/flat-union-no-base.exit >> create mode 100644 tests/qapi-schema/flat-union-no-base.json >> create mode 100644 tests/qapi-schema/flat-union-no-base.out >> create mode 100644 tests/qapi-schema/union-invalid-base.err >> create mode 100644 tests/qapi-schema/union-invalid-base.exit >> create mode 100644 tests/qapi-schema/union-invalid-base.json >> create mode 100644 tests/qapi-schema/union-invalid-base.out >> >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index 1954292..cea346f 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception): >> def __str__(self): >> return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg) >> >> +class QAPIExprError(Exception): >> + def __init__(self, expr_info, msg): >> + self.fp = expr_info['fp'] >> + self.line = expr_info['line'] >> + self.msg = msg >> + >> + def __str__(self): >> + return "%s:%s: %s" % (self.fp.name, self.line, self.msg) >> + >> class QAPISchema: >> >> def __init__(self, fp): >> @@ -64,7 +73,10 @@ class QAPISchema: >> self.accept() >> >> while self.tok != None: >> - self.exprs.append(self.get_expr(False)) >> + expr_info = {'fp': fp, 'line': self.line} >> + expr_elem = {'expr': self.get_expr(False), >> + 'info': expr_info} >> + self.exprs.append(expr_elem) >> >> def accept(self): >> while True: >> @@ -162,6 +174,89 @@ class QAPISchema: >> raise QAPISchemaError(self, 'Expected "{", "[" or string') >> return expr >> >> +def find_base_fields(base): >> + base_struct_define = find_struct(base) >> + if not base_struct_define: >> + return None >> + return base_struct_define['data'] >> + >> +# Return the discriminator enum define if discrminator is specified as an >> +# enum type, otherwise return None. >> +def discriminator_find_enum_define(expr): >> + base = expr.get('base') >> + discriminator = expr.get('discriminator') > Why not expr['base'] and expr['discriminator']? > > More of the same below. No need to respin just for that; we can clean > it up on top. > It is possible 'base' not present in some caller, so use .get to avoid python error. >> + >> + if not (discriminator and base): >> + return None >> + >> + base_fields = find_base_fields(base) >> + if not base_fields: >> + return None >> + >> + discriminator_type = base_fields.get(discriminator) >> + if not discriminator_type: >> + return None >> + >> + return find_enum(discriminator_type) >> + >> +def check_union(expr, expr_info): >> + name = expr['union'] >> + base = expr.get('base') >> + discriminator = expr.get('discriminator') >> + members = expr['data'] >> + >> + # If the object has a member 'base', its value must name a complex type. >> + if base: >> + base_fields = find_base_fields(base) >> + if not base_fields: >> + raise QAPIExprError(expr_info, >> + "Base '%s' is not a valid type" >> + % base) >> + >> + # If the union object has no member 'discriminator', it's an >> + # ordinary union. >> + if not discriminator: >> + enum_define = None >> + >> + # Else if the value of member 'discriminator' is {}, it's an >> + # anonymous union. >> + elif discriminator == {}: >> + enum_define = None >> + >> + # Else, it's a flat union. >> + else: >> + # The object must have a member 'base'. >> + if not base: >> + raise QAPIExprError(expr_info, >> + "Flat union '%s' must have a base field" >> + % name) >> + # The value of member 'discriminator' must name a member of the >> + # base type. >> + if not base_fields.get(discriminator): >> + raise QAPIExprError(expr_info, >> + "Discriminator '%s' is not a member of base " >> + "type '%s'" >> + % (discriminator, base)) >> + enum_define = discriminator_find_enum_define(expr) > Could this be simplified? Like this: > > # The value of member 'discriminator' must name a member of the > # base type. > discriminator_type = base_fields.get(discriminator): > if not discriminator_type > raise QAPIExprError(expr_info, > "Discriminator '%s' is not a member of base " > "type '%s'" > % (discriminator, base)) > enum_define = find_enum(discriminator_type) > > It's the only use of discriminator_find_enum_define()... > > Hmm, later patches add more uses, so my simplification is probably not > worthwhile. Anyway, we can simplify on top. > >> + >> + # Check every branch >> + for (key, value) in members.items(): >> + # If this named member's value names an enum type, then all members >> + # of 'data' must also be members of the enum type. >> + if enum_define and not key in enum_define['enum_values']: >> + raise QAPIExprError(expr_info, >> + "Discriminator value '%s' is not found in " >> + "enum '%s'" % >> + (key, enum_define["enum_name"])) >> + # Todo: put more check such as whether each value is valid, but it may >> + # need more functions to handle array case, so leave it now. > I'm not sure I get your comment. > Above only checks key, and I found it is possible to check every branch's value, so leaves a comments here. >> + >> +def check_exprs(schema): >> + for expr_elem in schema.exprs: >> + expr = expr_elem['expr'] >> + if expr.has_key('union'): >> + check_union(expr, expr_elem['info']) >> + >> def parse_schema(fp): >> try: >> schema = QAPISchema(fp) >> @@ -171,7 +266,8 @@ def parse_schema(fp): >> >> exprs = [] >> >> - for expr in schema.exprs: >> + for expr_elem in schema.exprs: >> + expr = expr_elem['expr'] >> if expr.has_key('enum'): >> add_enum(expr['enum'], expr['data']) >> elif expr.has_key('union'): >> @@ -181,6 +277,12 @@ def parse_schema(fp): >> add_struct(expr) >> exprs.append(expr) >> >> + try: >> + check_exprs(schema) >> + except QAPIExprError, e: >> + print >>sys.stderr, e >> + exit(1) >> + >> return exprs >> >> def parse_args(typeinfo): >> diff --git a/tests/Makefile b/tests/Makefile >> index dfe06eb..6ac9889 100644 >> --- a/tests/Makefile >> +++ b/tests/Makefile >> @@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ >> qapi-schema-test.json quoted-structural-chars.json \ >> trailing-comma-list.json trailing-comma-object.json \ >> unclosed-list.json unclosed-object.json unclosed-string.json \ >> - duplicate-key.json) >> + duplicate-key.json union-invalid-base.json flat-union-no-base.json \ >> + flat-union-invalid-discriminator.json \ >> + flat-union-invalid-branch-key.json) >> >> GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h >> >> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err >> new file mode 100644 >> index 0000000..1125caf >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err >> @@ -0,0 +1 @@ >> +<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum' >> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit >> new file mode 100644 >> index 0000000..d00491f >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit >> @@ -0,0 +1 @@ >> +1 >> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json >> new file mode 100644 >> index 0000000..a624282 >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.json >> @@ -0,0 +1,17 @@ >> +{ 'enum': 'TestEnum', >> + 'data': [ 'value1', 'value2' ] } >> + >> +{ 'type': 'TestBase', >> + 'data': { 'enum1': 'TestEnum' } } >> + >> +{ 'type': 'TestTypeA', >> + 'data': { 'string': 'str' } } >> + >> +{ 'type': 'TestTypeB', >> + 'data': { 'integer': 'int' } } >> + >> +{ 'union': 'TestUnion', >> + 'base': 'TestBase', >> + 'discriminator': 'enum1', >> + 'data': { 'value_wrong': 'TestTypeA', >> + 'value2': 'TestTypeB' } } >> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out >> new file mode 100644 >> index 0000000..e69de29 >> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err >> new file mode 100644 >> index 0000000..cad9dbf >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err >> @@ -0,0 +1 @@ >> +<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase' >> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit >> new file mode 100644 >> index 0000000..d00491f >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit >> @@ -0,0 +1 @@ >> +1 >> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json >> new file mode 100644 >> index 0000000..887157e >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.json >> @@ -0,0 +1,17 @@ >> +{ 'enum': 'TestEnum', >> + 'data': [ 'value1', 'value2' ] } >> + >> +{ 'type': 'TestBase', >> + 'data': { 'enum1': 'TestEnum' } } >> + >> +{ 'type': 'TestTypeA', >> + 'data': { 'string': 'str' } } >> + >> +{ 'type': 'TestTypeB', >> + 'data': { 'integer': 'int' } } >> + >> +{ 'union': 'TestUnion', >> + 'base': 'TestBase', >> + 'discriminator': 'enum_wrong', >> + 'data': { 'value1': 'TestTypeA', >> + 'value2': 'TestTypeB' } } >> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out >> new file mode 100644 >> index 0000000..e69de29 >> diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err >> new file mode 100644 >> index 0000000..e695315 >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-no-base.err >> @@ -0,0 +1 @@ >> +<stdin>:13: Flat union 'TestUnion' must have a base field >> diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit >> new file mode 100644 >> index 0000000..d00491f >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-no-base.exit >> @@ -0,0 +1 @@ >> +1 >> diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json >> new file mode 100644 >> index 0000000..e0900d4 >> --- /dev/null >> +++ b/tests/qapi-schema/flat-union-no-base.json >> @@ -0,0 +1,16 @@ >> +{ 'enum': 'TestEnum', >> + 'data': [ 'value1', 'value2' ] } >> + >> +{ 'type': 'TestBase', >> + 'data': { 'enum1': 'TestEnum' } } >> + > TestEnum and TestBase aren't used. > will remove. >> +{ 'type': 'TestTypeA', >> + 'data': { 'string': 'str' } } >> + >> +{ 'type': 'TestTypeB', >> + 'data': { 'integer': 'int' } } >> + >> +{ 'union': 'TestUnion', >> + 'discriminator': 'enum1', >> + 'data': { 'value1': 'TestTypeA', >> + 'value2': 'TestTypeB' } } >> diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out >> new file mode 100644 >> index 0000000..e69de29 >> diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err >> new file mode 100644 >> index 0000000..dd8e3d1 >> --- /dev/null >> +++ b/tests/qapi-schema/union-invalid-base.err >> @@ -0,0 +1 @@ >> +<stdin>:7: Base 'TestBaseWrong' is not a valid type >> diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit >> new file mode 100644 >> index 0000000..d00491f >> --- /dev/null >> +++ b/tests/qapi-schema/union-invalid-base.exit >> @@ -0,0 +1 @@ >> +1 >> diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json >> new file mode 100644 >> index 0000000..1fa4930 >> --- /dev/null >> +++ b/tests/qapi-schema/union-invalid-base.json >> @@ -0,0 +1,10 @@ >> +{ 'type': 'TestTypeA', >> + 'data': { 'string': 'str' } } >> + >> +{ 'type': 'TestTypeB', >> + 'data': { 'integer': 'int' } } >> + >> +{ 'union': 'TestUnion', >> + 'base': 'TestBaseWrong', >> + 'data': { 'value1': 'TestTypeA', >> + 'value2': 'TestTypeB' } } >> diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out >> new file mode 100644 >> index 0000000..e69de29 > Tests cover all the new errors. Good.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 1954292..cea346f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -50,6 +50,15 @@ class QAPISchemaError(Exception): def __str__(self): return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg) +class QAPIExprError(Exception): + def __init__(self, expr_info, msg): + self.fp = expr_info['fp'] + self.line = expr_info['line'] + self.msg = msg + + def __str__(self): + return "%s:%s: %s" % (self.fp.name, self.line, self.msg) + class QAPISchema: def __init__(self, fp): @@ -64,7 +73,10 @@ class QAPISchema: self.accept() while self.tok != None: - self.exprs.append(self.get_expr(False)) + expr_info = {'fp': fp, 'line': self.line} + expr_elem = {'expr': self.get_expr(False), + 'info': expr_info} + self.exprs.append(expr_elem) def accept(self): while True: @@ -162,6 +174,89 @@ class QAPISchema: raise QAPISchemaError(self, 'Expected "{", "[" or string') return expr +def find_base_fields(base): + base_struct_define = find_struct(base) + if not base_struct_define: + return None + return base_struct_define['data'] + +# Return the discriminator enum define if discrminator is specified as an +# enum type, otherwise return None. +def discriminator_find_enum_define(expr): + base = expr.get('base') + discriminator = expr.get('discriminator') + + if not (discriminator and base): + return None + + base_fields = find_base_fields(base) + if not base_fields: + return None + + discriminator_type = base_fields.get(discriminator) + if not discriminator_type: + return None + + return find_enum(discriminator_type) + +def check_union(expr, expr_info): + name = expr['union'] + base = expr.get('base') + discriminator = expr.get('discriminator') + members = expr['data'] + + # If the object has a member 'base', its value must name a complex type. + if base: + base_fields = find_base_fields(base) + if not base_fields: + raise QAPIExprError(expr_info, + "Base '%s' is not a valid type" + % base) + + # If the union object has no member 'discriminator', it's an + # ordinary union. + if not discriminator: + enum_define = None + + # Else if the value of member 'discriminator' is {}, it's an + # anonymous union. + elif discriminator == {}: + enum_define = None + + # Else, it's a flat union. + else: + # The object must have a member 'base'. + if not base: + raise QAPIExprError(expr_info, + "Flat union '%s' must have a base field" + % name) + # The value of member 'discriminator' must name a member of the + # base type. + if not base_fields.get(discriminator): + raise QAPIExprError(expr_info, + "Discriminator '%s' is not a member of base " + "type '%s'" + % (discriminator, base)) + enum_define = discriminator_find_enum_define(expr) + + # Check every branch + for (key, value) in members.items(): + # If this named member's value names an enum type, then all members + # of 'data' must also be members of the enum type. + if enum_define and not key in enum_define['enum_values']: + raise QAPIExprError(expr_info, + "Discriminator value '%s' is not found in " + "enum '%s'" % + (key, enum_define["enum_name"])) + # Todo: put more check such as whether each value is valid, but it may + # need more functions to handle array case, so leave it now. + +def check_exprs(schema): + for expr_elem in schema.exprs: + expr = expr_elem['expr'] + if expr.has_key('union'): + check_union(expr, expr_elem['info']) + def parse_schema(fp): try: schema = QAPISchema(fp) @@ -171,7 +266,8 @@ def parse_schema(fp): exprs = [] - for expr in schema.exprs: + for expr_elem in schema.exprs: + expr = expr_elem['expr'] if expr.has_key('enum'): add_enum(expr['enum'], expr['data']) elif expr.has_key('union'): @@ -181,6 +277,12 @@ def parse_schema(fp): add_struct(expr) exprs.append(expr) + try: + check_exprs(schema) + except QAPIExprError, e: + print >>sys.stderr, e + exit(1) + return exprs def parse_args(typeinfo): diff --git a/tests/Makefile b/tests/Makefile index dfe06eb..6ac9889 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ qapi-schema-test.json quoted-structural-chars.json \ trailing-comma-list.json trailing-comma-object.json \ unclosed-list.json unclosed-object.json unclosed-string.json \ - duplicate-key.json) + duplicate-key.json union-invalid-base.json flat-union-no-base.json \ + flat-union-invalid-discriminator.json \ + flat-union-invalid-branch-key.json) GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err new file mode 100644 index 0000000..1125caf --- /dev/null +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err @@ -0,0 +1 @@ +<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum' diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json new file mode 100644 index 0000000..a624282 --- /dev/null +++ b/tests/qapi-schema/flat-union-invalid-branch-key.json @@ -0,0 +1,17 @@ +{ 'enum': 'TestEnum', + 'data': [ 'value1', 'value2' ] } + +{ 'type': 'TestBase', + 'data': { 'enum1': 'TestEnum' } } + +{ 'type': 'TestTypeA', + 'data': { 'string': 'str' } } + +{ 'type': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': 'TestBase', + 'discriminator': 'enum1', + 'data': { 'value_wrong': 'TestTypeA', + 'value2': 'TestTypeB' } } diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err new file mode 100644 index 0000000..cad9dbf --- /dev/null +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err @@ -0,0 +1 @@ +<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase' diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json new file mode 100644 index 0000000..887157e --- /dev/null +++ b/tests/qapi-schema/flat-union-invalid-discriminator.json @@ -0,0 +1,17 @@ +{ 'enum': 'TestEnum', + 'data': [ 'value1', 'value2' ] } + +{ 'type': 'TestBase', + 'data': { 'enum1': 'TestEnum' } } + +{ 'type': 'TestTypeA', + 'data': { 'string': 'str' } } + +{ 'type': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': 'TestBase', + 'discriminator': 'enum_wrong', + 'data': { 'value1': 'TestTypeA', + 'value2': 'TestTypeB' } } diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err new file mode 100644 index 0000000..e695315 --- /dev/null +++ b/tests/qapi-schema/flat-union-no-base.err @@ -0,0 +1 @@ +<stdin>:13: Flat union 'TestUnion' must have a base field diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/flat-union-no-base.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json new file mode 100644 index 0000000..e0900d4 --- /dev/null +++ b/tests/qapi-schema/flat-union-no-base.json @@ -0,0 +1,16 @@ +{ 'enum': 'TestEnum', + 'data': [ 'value1', 'value2' ] } + +{ 'type': 'TestBase', + 'data': { 'enum1': 'TestEnum' } } + +{ 'type': 'TestTypeA', + 'data': { 'string': 'str' } } + +{ 'type': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'discriminator': 'enum1', + 'data': { 'value1': 'TestTypeA', + 'value2': 'TestTypeB' } } diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err new file mode 100644 index 0000000..dd8e3d1 --- /dev/null +++ b/tests/qapi-schema/union-invalid-base.err @@ -0,0 +1 @@ +<stdin>:7: Base 'TestBaseWrong' is not a valid type diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/union-invalid-base.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json new file mode 100644 index 0000000..1fa4930 --- /dev/null +++ b/tests/qapi-schema/union-invalid-base.json @@ -0,0 +1,10 @@ +{ 'type': 'TestTypeA', + 'data': { 'string': 'str' } } + +{ 'type': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': 'TestBaseWrong', + 'data': { 'value1': 'TestTypeA', + 'value2': 'TestTypeB' } }