From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33291) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbBT3-0008PW-GA for qemu-devel@nongnu.org; Thu, 26 Mar 2015 13:21:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbBSy-0003mZ-Uj for qemu-devel@nongnu.org; Thu, 26 Mar 2015 13:21:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55933) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbBSy-0003mK-Nm for qemu-devel@nongnu.org; Thu, 26 Mar 2015 13:21:12 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-17-git-send-email-eblake@redhat.com> Date: Thu, 26 Mar 2015 18:21:08 +0100 In-Reply-To: <1427227433-5030-17-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:41 -0600") Message-ID: <877fu3y8sb.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 16/28] qapi: Better error messages for duplicated expressions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com Eric Blake writes: > The previous commit demonstrated that the generator overlooked > duplicate expressions: > - a complex type or command reusing a built-in type name > - redeclaration of a type name, whether by the same or different > metatype > - redeclaration of a command or event > - collision of a type with implicit 'Kind' enum for a union > - collision with an implicit MAX enum constant, or with various > case spellings of events > > Since the c_type() function in the generator treats all names > as being in the same namespace, this patch adds a global array > to track all known names and their source, to prevent collisions > before it can cause further problems. 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 > --- > scripts/qapi.py | 68 +++++++++++++++++++++++++------- > tests/qapi-schema/bad-type-dict.err | 2 +- > tests/qapi-schema/command-int.err | 1 + > tests/qapi-schema/command-int.exit | 2 +- > tests/qapi-schema/command-int.json | 2 +- > tests/qapi-schema/command-int.out | 3 -- > tests/qapi-schema/enum-union-clash.err | 1 + > tests/qapi-schema/enum-union-clash.exit | 2 +- > tests/qapi-schema/enum-union-clash.json | 2 +- > tests/qapi-schema/enum-union-clash.out | 5 --- > tests/qapi-schema/event-case.err | 1 + > tests/qapi-schema/event-case.exit | 2 +- > tests/qapi-schema/event-case.json | 2 +- > tests/qapi-schema/event-case.out | 3 -- > tests/qapi-schema/event-max.err | 1 + > tests/qapi-schema/event-max.exit | 2 +- > tests/qapi-schema/event-max.json | 2 +- > tests/qapi-schema/event-max.out | 3 -- > tests/qapi-schema/redefined-builtin.err | 1 + > tests/qapi-schema/redefined-builtin.exit | 2 +- > tests/qapi-schema/redefined-builtin.json | 2 +- > tests/qapi-schema/redefined-builtin.out | 3 -- > tests/qapi-schema/redefined-command.err | 1 + > tests/qapi-schema/redefined-command.exit | 2 +- > tests/qapi-schema/redefined-command.json | 2 +- > tests/qapi-schema/redefined-command.out | 4 -- > tests/qapi-schema/redefined-event.err | 1 + > tests/qapi-schema/redefined-event.exit | 2 +- > tests/qapi-schema/redefined-event.json | 2 +- > tests/qapi-schema/redefined-event.out | 4 -- > tests/qapi-schema/redefined-type.err | 1 + > tests/qapi-schema/redefined-type.exit | 2 +- > tests/qapi-schema/redefined-type.json | 2 +- > tests/qapi-schema/redefined-type.out | 4 -- > 34 files changed, 78 insertions(+), 61 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 90eb3bc..5d0dc91 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -32,6 +32,12 @@ builtin_types = { > 'size': 'QTYPE_QINT', > } > > +enum_types = [] > +struct_types = [] > +union_types = [] > +events = [] > +all_names = {} > + > def error_path(parent): > res = "" > while parent: > @@ -256,7 +262,17 @@ def discriminator_find_enum_define(expr): > return find_enum(discriminator_type) > > def check_event(expr, expr_info): > + global events > + name = expr['event'] > params = expr.get('data') > + > + if name == 'MAX': > + raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created") > + if name != name.upper(): > + raise QAPIExprError(expr_info, "Event name '%s' should be upper case" > + % name) > + events.append(name) > + > if params: > for argname, argentry, optional, structured in parse_args(params): > if structured: > @@ -408,7 +424,7 @@ def check_keys(expr_elem, meta, required, optional=[]): > name = expr[meta] > if not isinstance(name, str): > raise QAPIExprError(info, > - "%s key must have a string value" % meta) > + "'%s' key must have a string value" % meta) > required = required + [ meta ] > for (key, value) in expr.items(): > if not key in required and not key in optional: Let's squash this hunk into PATCH 14. > @@ -423,6 +439,9 @@ def check_keys(expr_elem, meta, required, optional=[]): > > > def parse_schema(input_file): > + global all_names > + exprs = [] > + > # First pass: read entire file into memory > try: > schema = QAPISchema(open(input_file, "r")) > @@ -430,30 +449,34 @@ def parse_schema(input_file): > print >>sys.stderr, e > exit(1) > > - exprs = [] > - > try: > # Next pass: learn the types and check for valid expression keys. At > # this point, top-level 'include' has already been flattened. > + for builtin in builtin_types.keys(): > + all_names[builtin] = 'built-in' > for expr_elem in schema.exprs: > expr = expr_elem['expr'] > + info = expr_elem['info'] > if expr.has_key('enum'): > check_keys(expr_elem, 'enum', ['data']) > - add_enum(expr['enum'], expr['data']) > + add_enum(expr['enum'], info, expr['data']) > elif expr.has_key('union'): > check_keys(expr_elem, 'union', ['data'], > ['base', 'discriminator']) > - add_union(expr) > + add_union(expr, info) > elif expr.has_key('alternate'): > check_keys(expr_elem, 'alternate', ['data']) > + add_name(expr['alternate'], info, 'alternate') > elif expr.has_key('type'): > check_keys(expr_elem, 'type', ['data'], ['base']) > - add_struct(expr) > + add_struct(expr, info) > elif expr.has_key('command'): > check_keys(expr_elem, 'command', [], > ['data', 'returns', 'gen', 'success-response']) > + add_name(expr['command'], info, 'command') > elif expr.has_key('event'): > check_keys(expr_elem, 'event', [], ['data']) > + add_name(expr['event'], info, 'event') > else: > raise QAPIExprError(expr_elem['info'], > "Expression is missing metatype") > @@ -464,9 +487,11 @@ def parse_schema(input_file): > expr = expr_elem['expr'] > if expr.has_key('union'): > if not discriminator_find_enum_define(expr): > - add_enum('%sKind' % expr['union']) > + add_enum('%sKind' % expr['union'], expr_elem['info'], > + implicit=True) > elif expr.has_key('alternate'): > - add_enum('%sKind' % expr['alternate']) > + add_enum('%sKind' % expr['alternate'], expr_elem['info'], > + implicit=True) > > # Final pass - validate that exprs make sense > check_exprs(schema) > @@ -560,12 +585,22 @@ def type_name(name): > return c_list_type(name[0]) > return name > > -enum_types = [] > -struct_types = [] > -union_types = [] > +def add_name(name, info, meta, implicit = False): > + global all_names > + if name in all_names: > + raise QAPIExprError(info, > + "%s '%s' is already defined" > + %(all_names[name], name)) > + if not implicit and name[-4:] == 'Kind': > + raise QAPIExprError(info, > + "%s '%s' should not end in 'Kind'" > + %(meta, name)) > + all_names[name] = meta > > -def add_struct(definition): > +def add_struct(definition, info): > global struct_types > + name = definition['type'] > + add_name(name, info, 'struct') > struct_types.append(definition) > > def find_struct(name): > @@ -575,8 +610,10 @@ def find_struct(name): > return struct > return None > > -def add_union(definition): > +def add_union(definition, info): > global union_types > + name = definition['union'] > + add_name(name, info, 'union') > union_types.append(definition) > > def find_union(name): > @@ -586,8 +623,9 @@ def find_union(name): > return union > return None > > -def add_enum(name, enum_values = None): > +def add_enum(name, info, enum_values = None, implicit = False): > global enum_types > + add_name(name, info, 'enum', implicit) > enum_types.append({"enum_name": name, "enum_values": enum_values}) > > def find_enum(name): > @@ -629,7 +667,7 @@ def c_type(name, is_param=False): > return name > elif name == None or len(name) == 0: > return 'void' > - elif name == name.upper(): > + elif name in events: > return '%sEvent *%s' % (camel_case(name), eatspace) > else: > return '%s *%s' % (name, eatspace) One instance kitten-killing grossness down :) [Tests look good...] Since the misplaced hunk is just polish: Reviewed-by: Markus Armbruster