From 4dc2e6906e1084fdd37bf67385c5dcd2c72ae22b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:17 -0600 Subject: [PATCH] qapi: Better error messages for duplicated expressions 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 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 Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 63 ++++++++++++++++++------ 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-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 -- 29 files changed, 70 insertions(+), 54 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 868f08b593..6a339d60b5 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,14 @@ 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.upper() == 'MAX': + raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created") + events.append(name) + if params: for argname, argentry, optional, structured in parse_args(params): if structured: @@ -430,6 +443,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")) @@ -437,30 +453,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") @@ -471,9 +491,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) @@ -567,12 +589,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): @@ -582,8 +614,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): @@ -593,8 +627,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): @@ -636,7 +671,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) diff --git a/tests/qapi-schema/command-int.err b/tests/qapi-schema/command-int.err index e69de29bb2..0f9300679b 100644 --- a/tests/qapi-schema/command-int.err +++ b/tests/qapi-schema/command-int.err @@ -0,0 +1 @@ +tests/qapi-schema/command-int.json:2: built-in 'int' is already defined diff --git a/tests/qapi-schema/command-int.exit b/tests/qapi-schema/command-int.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/command-int.exit +++ b/tests/qapi-schema/command-int.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/command-int.json b/tests/qapi-schema/command-int.json index fcbb643c44..c90d408abe 100644 --- a/tests/qapi-schema/command-int.json +++ b/tests/qapi-schema/command-int.json @@ -1,3 +1,3 @@ -# FIXME: we should reject collisions between commands and types +# we reject collisions between commands and types { 'command': 'int', 'data': { 'character': 'str' }, 'returns': { 'value': 'int' } } diff --git a/tests/qapi-schema/command-int.out b/tests/qapi-schema/command-int.out index d8e1854a04..e69de29bb2 100644 --- a/tests/qapi-schema/command-int.out +++ b/tests/qapi-schema/command-int.out @@ -1,3 +0,0 @@ -[OrderedDict([('command', 'int'), ('data', OrderedDict([('character', 'str')])), ('returns', OrderedDict([('value', 'int')]))])] -[] -[] diff --git a/tests/qapi-schema/enum-union-clash.err b/tests/qapi-schema/enum-union-clash.err index e69de29bb2..c04e1a8064 100644 --- a/tests/qapi-schema/enum-union-clash.err +++ b/tests/qapi-schema/enum-union-clash.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-union-clash.json:2: enum 'UnionKind' should not end in 'Kind' diff --git a/tests/qapi-schema/enum-union-clash.exit b/tests/qapi-schema/enum-union-clash.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/enum-union-clash.exit +++ b/tests/qapi-schema/enum-union-clash.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-union-clash.json b/tests/qapi-schema/enum-union-clash.json index 714ff6d556..593282b6cf 100644 --- a/tests/qapi-schema/enum-union-clash.json +++ b/tests/qapi-schema/enum-union-clash.json @@ -1,4 +1,4 @@ -# FIXME: we should reject types that would conflict with implicit union enum +# we reject types that would conflict with implicit union enum { 'enum': 'UnionKind', 'data': [ 'oops' ] } { 'union': 'Union', 'data': { 'a': 'int' } } diff --git a/tests/qapi-schema/enum-union-clash.out b/tests/qapi-schema/enum-union-clash.out index d45f5e8a69..e69de29bb2 100644 --- a/tests/qapi-schema/enum-union-clash.out +++ b/tests/qapi-schema/enum-union-clash.out @@ -1,5 +0,0 @@ -[OrderedDict([('enum', 'UnionKind'), ('data', ['oops'])]), - OrderedDict([('union', 'Union'), ('data', OrderedDict([('a', 'int')]))])] -[{'enum_name': 'UnionKind', 'enum_values': ['oops']}, - {'enum_name': 'UnionKind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/event-max.err b/tests/qapi-schema/event-max.err index e69de29bb2..c856534379 100644 --- a/tests/qapi-schema/event-max.err +++ b/tests/qapi-schema/event-max.err @@ -0,0 +1 @@ +tests/qapi-schema/event-max.json:2: Event name 'MAX' cannot be created diff --git a/tests/qapi-schema/event-max.exit b/tests/qapi-schema/event-max.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/event-max.exit +++ b/tests/qapi-schema/event-max.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/event-max.json b/tests/qapi-schema/event-max.json index 997c61c511..f3d7de2a30 100644 --- a/tests/qapi-schema/event-max.json +++ b/tests/qapi-schema/event-max.json @@ -1,2 +1,2 @@ -# FIXME: an event named 'MAX' would conflict with implicit C enum +# an event named 'MAX' would conflict with implicit C enum { 'event': 'MAX' } diff --git a/tests/qapi-schema/event-max.out b/tests/qapi-schema/event-max.out index 010c42b3a9..e69de29bb2 100644 --- a/tests/qapi-schema/event-max.out +++ b/tests/qapi-schema/event-max.out @@ -1,3 +0,0 @@ -[OrderedDict([('event', 'MAX')])] -[] -[] diff --git a/tests/qapi-schema/redefined-builtin.err b/tests/qapi-schema/redefined-builtin.err index e69de29bb2..b2757225c4 100644 --- a/tests/qapi-schema/redefined-builtin.err +++ b/tests/qapi-schema/redefined-builtin.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-builtin.json:2: built-in 'size' is already defined diff --git a/tests/qapi-schema/redefined-builtin.exit b/tests/qapi-schema/redefined-builtin.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/redefined-builtin.exit +++ b/tests/qapi-schema/redefined-builtin.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-builtin.json b/tests/qapi-schema/redefined-builtin.json index a10050dc73..df328ccc66 100644 --- a/tests/qapi-schema/redefined-builtin.json +++ b/tests/qapi-schema/redefined-builtin.json @@ -1,2 +1,2 @@ -# FIXME: we should reject types that duplicate builtin names +# we reject types that duplicate builtin names { 'type': 'size', 'data': { 'myint': 'size' } } diff --git a/tests/qapi-schema/redefined-builtin.out b/tests/qapi-schema/redefined-builtin.out index b0a9aea548..e69de29bb2 100644 --- a/tests/qapi-schema/redefined-builtin.out +++ b/tests/qapi-schema/redefined-builtin.out @@ -1,3 +0,0 @@ -[OrderedDict([('type', 'size'), ('data', OrderedDict([('myint', 'size')]))])] -[] -[OrderedDict([('type', 'size'), ('data', OrderedDict([('myint', 'size')]))])] diff --git a/tests/qapi-schema/redefined-command.err b/tests/qapi-schema/redefined-command.err index e69de29bb2..82ae256e63 100644 --- a/tests/qapi-schema/redefined-command.err +++ b/tests/qapi-schema/redefined-command.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-command.json:3: command 'foo' is already defined diff --git a/tests/qapi-schema/redefined-command.exit b/tests/qapi-schema/redefined-command.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/redefined-command.exit +++ b/tests/qapi-schema/redefined-command.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-command.json b/tests/qapi-schema/redefined-command.json index d8c9975e1c..247e401948 100644 --- a/tests/qapi-schema/redefined-command.json +++ b/tests/qapi-schema/redefined-command.json @@ -1,3 +1,3 @@ -# FIXME: we should reject commands defined more than once +# we reject commands defined more than once { 'command': 'foo', 'data': { 'one': 'str' } } { 'command': 'foo', 'data': { '*two': 'str' } } diff --git a/tests/qapi-schema/redefined-command.out b/tests/qapi-schema/redefined-command.out index 44ddba60d5..e69de29bb2 100644 --- a/tests/qapi-schema/redefined-command.out +++ b/tests/qapi-schema/redefined-command.out @@ -1,4 +0,0 @@ -[OrderedDict([('command', 'foo'), ('data', OrderedDict([('one', 'str')]))]), - OrderedDict([('command', 'foo'), ('data', OrderedDict([('*two', 'str')]))])] -[] -[] diff --git a/tests/qapi-schema/redefined-event.err b/tests/qapi-schema/redefined-event.err index e69de29bb2..35429cb481 100644 --- a/tests/qapi-schema/redefined-event.err +++ b/tests/qapi-schema/redefined-event.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-event.json:3: event 'EVENT_A' is already defined diff --git a/tests/qapi-schema/redefined-event.exit b/tests/qapi-schema/redefined-event.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/redefined-event.exit +++ b/tests/qapi-schema/redefined-event.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-event.json b/tests/qapi-schema/redefined-event.json index 152cce720f..7717e91c18 100644 --- a/tests/qapi-schema/redefined-event.json +++ b/tests/qapi-schema/redefined-event.json @@ -1,3 +1,3 @@ -# FIXME: we should reject duplicate events +# we reject duplicate events { 'event': 'EVENT_A', 'data': { 'myint': 'int' } } { 'event': 'EVENT_A', 'data': { 'myint': 'int' } } diff --git a/tests/qapi-schema/redefined-event.out b/tests/qapi-schema/redefined-event.out index 261b183fc9..e69de29bb2 100644 --- a/tests/qapi-schema/redefined-event.out +++ b/tests/qapi-schema/redefined-event.out @@ -1,4 +0,0 @@ -[OrderedDict([('event', 'EVENT_A'), ('data', OrderedDict([('myint', 'int')]))]), - OrderedDict([('event', 'EVENT_A'), ('data', OrderedDict([('myint', 'int')]))])] -[] -[] diff --git a/tests/qapi-schema/redefined-type.err b/tests/qapi-schema/redefined-type.err index e69de29bb2..06ea78c478 100644 --- a/tests/qapi-schema/redefined-type.err +++ b/tests/qapi-schema/redefined-type.err @@ -0,0 +1 @@ +tests/qapi-schema/redefined-type.json:3: struct 'foo' is already defined diff --git a/tests/qapi-schema/redefined-type.exit b/tests/qapi-schema/redefined-type.exit index 573541ac97..d00491fd7e 100644 --- a/tests/qapi-schema/redefined-type.exit +++ b/tests/qapi-schema/redefined-type.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/redefined-type.json b/tests/qapi-schema/redefined-type.json index 7972194824..e6a5f24ca9 100644 --- a/tests/qapi-schema/redefined-type.json +++ b/tests/qapi-schema/redefined-type.json @@ -1,3 +1,3 @@ -# FIXME: we should reject types defined more than once +# we reject types defined more than once { 'type': 'foo', 'data': { 'one': 'str' } } { 'enum': 'foo', 'data': [ 'two' ] } diff --git a/tests/qapi-schema/redefined-type.out b/tests/qapi-schema/redefined-type.out index b509e5776c..e69de29bb2 100644 --- a/tests/qapi-schema/redefined-type.out +++ b/tests/qapi-schema/redefined-type.out @@ -1,4 +0,0 @@ -[OrderedDict([('type', 'foo'), ('data', OrderedDict([('one', 'str')]))]), - OrderedDict([('enum', 'foo'), ('data', ['two'])])] -[{'enum_name': 'foo', 'enum_values': ['two']}] -[OrderedDict([('type', 'foo'), ('data', OrderedDict([('one', 'str')]))])] -- GitLab