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 868f08b59..6a339d60b 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 e69de29bb..0f9300679 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 573541ac9..d00491fd7 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 fcbb643c4..c90d408ab 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 d8e1854a0..e69de29bb 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 e69de29bb..c04e1a806 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 573541ac9..d00491fd7 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 714ff6d55..593282b6c 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 d45f5e8a6..e69de29bb 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 e69de29bb..c85653437 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 573541ac9..d00491fd7 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 997c61c51..f3d7de2a3 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 010c42b3a..e69de29bb 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 e69de29bb..b2757225c 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 573541ac9..d00491fd7 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 a10050dc7..df328ccc6 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 b0a9aea54..e69de29bb 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 e69de29bb..82ae256e6 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 573541ac9..d00491fd7 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 d8c9975e1..247e40194 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 44ddba60d..e69de29bb 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 e69de29bb..35429cb48 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 573541ac9..d00491fd7 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 152cce720..7717e91c1 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 261b183fc..e69de29bb 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 e69de29bb..06ea78c47 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 573541ac9..d00491fd7 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 797219482..e6a5f24ca 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 b509e5776..e69de29bb 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')]))])] -- 2.39.5