From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKqju-0003UH-Bm for qemu-devel@nongnu.org; Tue, 04 Mar 2014 09:54:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKqji-0006J8-2I for qemu-devel@nongnu.org; Tue, 04 Mar 2014 09:54:38 -0500 Received: from mail-pa0-x22e.google.com ([2607:f8b0:400e:c03::22e]:63464) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKqjh-0006I7-Nx for qemu-devel@nongnu.org; Tue, 04 Mar 2014 09:54:26 -0500 Received: by mail-pa0-f46.google.com with SMTP id kp14so5305550pab.33 for ; Tue, 04 Mar 2014 06:54:24 -0800 (PST) Message-ID: <5315E917.10707@gmail.com> Date: Tue, 04 Mar 2014 22:54:15 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1393499376-4374-1-git-send-email-wenchaoqemu@gmail.com> <1393499376-4374-5-git-send-email-wenchaoqemu@gmail.com> <8761nuqgfo.fsf@blackfin.pond.sub.org> In-Reply-To: <8761nuqgfo.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, Wenchao Xia , lcapitulino@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com ÓÚ 2014/3/4 20:47, Markus Armbruster дµÀ: > Wenchao Xia writes: > >> From: Wenchao Xia > 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 >> Signed-off-by: Wenchao Xia >> --- >> 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 @@ >> +: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 @@ >> +: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 @@ >> +: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 @@ >> +: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.