From b52c4b9cf0bbafdf8cede4ea1f62770d86815718 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:00 -0600 Subject: qapi: Simplify builtin type handling There was some redundancy between builtin_types[] and builtin_type_qtypes{}. Merge them into one. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 77d46aa..2b5775d 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -16,13 +16,7 @@ from ordereddict import OrderedDict import os import sys -builtin_types = [ - 'str', 'int', 'number', 'bool', - 'int8', 'int16', 'int32', 'int64', - 'uint8', 'uint16', 'uint32', 'uint64' -] - -builtin_type_qtypes = { +builtin_types = { 'str': 'QTYPE_QSTRING', 'int': 'QTYPE_QINT', 'number': 'QTYPE_QFLOAT', -- cgit v1.1 From cb17f79eef0d161e81ac457e4c1f124405be2a18 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:01 -0600 Subject: qapi: Fix generation of 'size' builtin type We were missing the 'size' builtin type (which means that QAPI using [ 'size' ] would fail to compile). Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 1 + 1 file changed, 1 insertion(+) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 2b5775d..d470347 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -29,6 +29,7 @@ builtin_types = { 'uint16': 'QTYPE_QINT', 'uint32': 'QTYPE_QINT', 'uint64': 'QTYPE_QINT', + 'size': 'QTYPE_QINT', } def error_path(parent): -- cgit v1.1 From fe2a9303c9e511462f662a415c2e9d2defe9b7ca Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:02 -0600 Subject: qapi: Require ASCII in schema Python 2 and Python 3 have a wild history of whether strings default to ascii or unicode, where Python 3 requires checking isinstance(foo, basestr) to cover all strings, but where that code is not portable to Python 2. It's simpler to just state that we don't care about Unicode strings, and to just always use the simpler isinstance(foo, str) everywhere. I'm no python expert, so I'm basing it on this conversation: https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg05278.html Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index d470347..20ee505 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -2,7 +2,7 @@ # QAPI helper library # # Copyright IBM, Corp. 2011 -# Copyright (c) 2013 Red Hat Inc. +# Copyright (c) 2013-2015 Red Hat Inc. # # Authors: # Anthony Liguori @@ -354,7 +354,7 @@ def parse_schema(input_file): return exprs def parse_args(typeinfo): - if isinstance(typeinfo, basestring): + if isinstance(typeinfo, str): struct = find_struct(typeinfo) assert struct != None typeinfo = struct['data'] -- cgit v1.1 From cf3935907b5df16f667d54ad6761c7e937dcf425 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:04 -0600 Subject: qapi: Better error messages for bad enums The previous commit demonstrated that the generator had several flaws with less-than-perfect enums: - an enum that listed the same string twice (or two variant strings that map to the same C enumerator) ended up generating an invalid C enum - because the generator adds a _MAX terminator to each enum, the use of an enum member 'max' can also cause this clash - if an enum omits 'data', the generator left a python stack trace rather than a graceful message - an enum that used a non-array 'data' was silently accepted by the parser - an enum that used non-string members in the 'data' member was silently accepted by the parser Add check_enum to cover these situations, and update testcases to match. 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 | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 20ee505..3ce8c33 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -311,13 +311,37 @@ def check_union(expr, expr_info): # Todo: add checking for values. Key is checked as above, value can be # also checked here, but we need more functions to handle array case. +def check_enum(expr, expr_info): + name = expr['enum'] + members = expr.get('data') + values = { 'MAX': '(automatic)' } + + if not isinstance(members, list): + raise QAPIExprError(expr_info, + "Enum '%s' requires an array for 'data'" % name) + for member in members: + if not isinstance(member, str): + raise QAPIExprError(expr_info, + "Enum '%s' member '%s' is not a string" + % (name, member)) + key = _generate_enum_string(member) + if key in values: + raise QAPIExprError(expr_info, + "Enum '%s' member '%s' clashes with '%s'" + % (name, member, values[key])) + values[key] = member + 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']) - if expr.has_key('event'): - check_event(expr, expr_elem['info']) + info = expr_elem['info'] + + if expr.has_key('enum'): + check_enum(expr, info) + elif expr.has_key('union'): + check_union(expr, info) + elif expr.has_key('event'): + check_event(expr, info) def parse_schema(input_file): try: @@ -331,7 +355,7 @@ def parse_schema(input_file): for expr_elem in schema.exprs: expr = expr_elem['expr'] if expr.has_key('enum'): - add_enum(expr['enum'], expr['data']) + add_enum(expr['enum'], expr.get('data')) elif expr.has_key('union'): add_union(expr) elif expr.has_key('type'): -- cgit v1.1 From a8d4a2e4d7e1a0207699de47142c9bdbf2cc8675 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:07 -0600 Subject: qapi: Forbid base without discriminator in unions None of the existing QMP or QGA interfaces uses a union with a base type but no discriminator; it is easier to avoid this in the generator to save room for other future extensions more likely to be useful. An earlier commit added a union-base-no-discriminator test to ensure that we eventually give a decent error message; likewise, removing UserDefUnion outright is okay, because we moved all the tests we wish to keep into the tests of the simple union UserDefNativeListUnion in the previous commit. Now is the time to actually forbid simple union with base, and remove the last vestiges from the testsuite. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 3ce8c33..438468e 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -259,22 +259,22 @@ def check_union(expr, expr_info): discriminator = expr.get('discriminator') members = expr['data'] - # If the object has a member 'base', its value must name a complex type. - if base: + # If the object has a member 'base', its value must name a complex type, + # and there must be a discriminator. + if base is not None: + if discriminator is None: + raise QAPIExprError(expr_info, + "Union '%s' requires a discriminator to go " + "along with base" %name) 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 == {}: + # If the union object has no member 'discriminator', it's a + # simple union. If 'discriminator' is {}, it is an anonymous union. + if not discriminator or discriminator == {}: enum_define = None # Else, it's a flat union. -- cgit v1.1 From 44bd1276a7dea747c41f250cb71ab65965343a7f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:08 -0600 Subject: qapi: Tighten checking of unions Previous commits demonstrated that the generator had several flaws with less-than-perfect unions: - a simple union that listed the same branch twice (or two variant names that map to the same C enumerator, including the implicit MAX sentinel) ended up generating invalid C code - an anonymous union that listed two branches with the same qtype ended up generating invalid C code - the generator crashed on anonymous union attempts to use an array type - the generator was silently ignoring a base type for anonymous unions - the generator allowed unknown types or nested anonymous unions as a branch in an anonymous union Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 89 ++++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 72 insertions(+), 17 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 438468e..5f0f699 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -224,6 +224,23 @@ def find_base_fields(base): return None return base_struct_define['data'] +# Return the qtype of an anonymous union branch, or None on error. +def find_anonymous_member_qtype(qapi_type): + if builtin_types.has_key(qapi_type): + return builtin_types[qapi_type] + elif find_struct(qapi_type): + return "QTYPE_QDICT" + elif find_enum(qapi_type): + return "QTYPE_QSTRING" + else: + union = find_union(qapi_type) + if union: + discriminator = union.get('discriminator') + if discriminator == {}: + return None + return "QTYPE_QDICT" + return None + # Return the discriminator enum define if discriminator is specified as an # enum type, otherwise return None. def discriminator_find_enum_define(expr): @@ -258,6 +275,8 @@ def check_union(expr, expr_info): base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] + values = { 'MAX': '(automatic)' } + types_seen = {} # If the object has a member 'base', its value must name a complex type, # and there must be a discriminator. @@ -266,26 +285,35 @@ def check_union(expr, expr_info): raise QAPIExprError(expr_info, "Union '%s' requires a discriminator to go " "along with base" %name) - 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 a # simple union. If 'discriminator' is {}, it is an anonymous union. - if not discriminator or discriminator == {}: + if discriminator is None or discriminator == {}: enum_define = None + if base is not None: + raise QAPIExprError(expr_info, + "Union '%s' must not have a base" + % name) # Else, it's a flat union. else: - # The object must have a member 'base'. - if not base: + # The object must have a string member 'base'. + if not isinstance(base, str): raise QAPIExprError(expr_info, - "Flat union '%s' must have a base field" + "Flat union '%s' must have a string base field" % name) + base_fields = find_base_fields(base) + if not base_fields: + raise QAPIExprError(expr_info, + "Base '%s' is not a valid type" + % base) + # The value of member 'discriminator' must name a member of the # base type. + if not isinstance(discriminator, str): + raise QAPIExprError(expr_info, + "Flat union '%s' discriminator must be a string" + % name) discriminator_type = base_fields.get(discriminator) if not discriminator_type: raise QAPIExprError(expr_info, @@ -301,15 +329,42 @@ def check_union(expr, expr_info): # Check every branch for (key, value) in members.items(): - # If this named member's value names an enum type, then all members + # If the discriminator 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: add checking for values. Key is checked as above, value can be - # also checked here, but we need more functions to handle array case. + if enum_define: + if 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"])) + + # Otherwise, check for conflicts in the generated enum + else: + c_key = _generate_enum_string(key) + if c_key in values: + raise QAPIExprError(expr_info, + "Union '%s' member '%s' clashes with '%s'" + % (name, key, values[c_key])) + values[c_key] = key + + # Ensure anonymous unions have no type conflicts. + if discriminator == {}: + if isinstance(value, list): + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' must " + "not be array type" % (name, key)) + qtype = find_anonymous_member_qtype(value) + if not qtype: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' has " + "invalid type '%s'" % (name, key, value)) + if qtype in types_seen: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' can't " + "be distinguished from member '%s'" + % (name, key, types_seen[qtype])) + types_seen[qtype] = key + def check_enum(expr, expr_info): name = expr['enum'] -- cgit v1.1 From 268a1c5eb10832c2e4476d3fe199ea547dabecb7 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:09 -0600 Subject: qapi: Prepare for catching more semantic parse errors This patch widens the scope of a try block (with the attending reindentation required by Python) in preparation for a future patch adding more instances of QAPIExprError inside the block. It's easier to separate indentation from semantic changes, so this patch has no real behavior change. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 5f0f699..0c3459b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -399,6 +399,7 @@ def check_exprs(schema): check_event(expr, info) def parse_schema(input_file): + # First pass: read entire file into memory try: schema = QAPISchema(open(input_file, "r")) except (QAPISchemaError, QAPIExprError), e: @@ -407,24 +408,26 @@ def parse_schema(input_file): exprs = [] - for expr_elem in schema.exprs: - expr = expr_elem['expr'] - if expr.has_key('enum'): - add_enum(expr['enum'], expr.get('data')) - elif expr.has_key('union'): - add_union(expr) - elif expr.has_key('type'): - add_struct(expr) - exprs.append(expr) - - # Try again for hidden UnionKind enum - for expr_elem in schema.exprs: - expr = expr_elem['expr'] - if expr.has_key('union'): - if not discriminator_find_enum_define(expr): - add_enum('%sKind' % expr['union']) - try: + # Next pass: learn the types. + for expr_elem in schema.exprs: + expr = expr_elem['expr'] + if expr.has_key('enum'): + add_enum(expr['enum'], expr.get('data')) + elif expr.has_key('union'): + add_union(expr) + elif expr.has_key('type'): + add_struct(expr) + exprs.append(expr) + + # Try again for hidden UnionKind enum + for expr_elem in schema.exprs: + expr = expr_elem['expr'] + if expr.has_key('union'): + if not discriminator_find_enum_define(expr): + add_enum('%sKind' % expr['union']) + + # Final pass - validate that exprs make sense check_exprs(schema) except QAPIExprError, e: print >>sys.stderr, e -- cgit v1.1 From 811d04fd0cff1229480d3f5b2e349f646ab6e3c1 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:10 -0600 Subject: qapi: Segregate anonymous unions into alternates in generator Special-casing 'discriminator == {}' for handling anonymous unions is getting awkward; since this particular type is not always a dictionary on the wire, it is easier to treat it as a completely different class of type, "alternate", so that if a type is listed in the union_types array, we know it is not an anonymous union. This patch just further segregates union handling, to make sure that anonymous unions are not stored in union_types, and splitting up check_union() into separate functions. A future patch will change the qapi grammar, and having the segregation already in place will make it easier to deal with the distinct meta-type. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 88 +++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 33 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 0c3459b..0b88325 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -224,21 +224,16 @@ def find_base_fields(base): return None return base_struct_define['data'] -# Return the qtype of an anonymous union branch, or None on error. -def find_anonymous_member_qtype(qapi_type): +# Return the qtype of an alternate branch, or None on error. +def find_alternate_member_qtype(qapi_type): if builtin_types.has_key(qapi_type): return builtin_types[qapi_type] elif find_struct(qapi_type): return "QTYPE_QDICT" elif find_enum(qapi_type): return "QTYPE_QSTRING" - else: - union = find_union(qapi_type) - if union: - discriminator = union.get('discriminator') - if discriminator == {}: - return None - return "QTYPE_QDICT" + elif find_union(qapi_type): + return "QTYPE_QDICT" return None # Return the discriminator enum define if discriminator is specified as an @@ -276,7 +271,6 @@ def check_union(expr, expr_info): discriminator = expr.get('discriminator') members = expr['data'] values = { 'MAX': '(automatic)' } - types_seen = {} # If the object has a member 'base', its value must name a complex type, # and there must be a discriminator. @@ -286,13 +280,15 @@ def check_union(expr, expr_info): "Union '%s' requires a discriminator to go " "along with base" %name) - # If the union object has no member 'discriminator', it's a - # simple union. If 'discriminator' is {}, it is an anonymous union. - if discriminator is None or discriminator == {}: + # Two types of unions, determined by discriminator. + assert discriminator != {} + + # With no discriminator it is a simple union. + if discriminator is None: enum_define = None if base is not None: raise QAPIExprError(expr_info, - "Union '%s' must not have a base" + "Simple union '%s' must not have a base" % name) # Else, it's a flat union. @@ -347,24 +343,46 @@ def check_union(expr, expr_info): % (name, key, values[c_key])) values[c_key] = key - # Ensure anonymous unions have no type conflicts. - if discriminator == {}: - if isinstance(value, list): - raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' must " - "not be array type" % (name, key)) - qtype = find_anonymous_member_qtype(value) - if not qtype: - raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' has " - "invalid type '%s'" % (name, key, value)) - if qtype in types_seen: - raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' can't " - "be distinguished from member '%s'" - % (name, key, types_seen[qtype])) - types_seen[qtype] = key +def check_alternate(expr, expr_info): + name = expr['union'] + base = expr.get('base') + discriminator = expr.get('discriminator') + members = expr['data'] + values = { 'MAX': '(automatic)' } + types_seen = {} + + assert discriminator == {} + if base is not None: + raise QAPIExprError(expr_info, + "Anonymous union '%s' must not have a base" + % name) + + # Check every branch + for (key, value) in members.items(): + # Check for conflicts in the generated enum + c_key = _generate_enum_string(key) + if c_key in values: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' clashes " + "with '%s'" % (name, key, values[c_key])) + values[c_key] = key + # Ensure alternates have no type conflicts. + if isinstance(value, list): + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' must " + "not be array type" % (name, key)) + qtype = find_alternate_member_qtype(value) + if not qtype: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' has " + "invalid type '%s'" % (name, key, value)) + if qtype in types_seen: + raise QAPIExprError(expr_info, + "Anonymous union '%s' member '%s' can't " + "be distinguished from member '%s'" + % (name, key, types_seen[qtype])) + types_seen[qtype] = key def check_enum(expr, expr_info): name = expr['enum'] @@ -394,7 +412,10 @@ def check_exprs(schema): if expr.has_key('enum'): check_enum(expr, info) elif expr.has_key('union'): - check_union(expr, info) + if expr.get('discriminator') == {}: + check_alternate(expr, info) + else: + check_union(expr, info) elif expr.has_key('event'): check_event(expr, info) @@ -536,7 +557,8 @@ def find_struct(name): def add_union(definition): global union_types - union_types.append(definition) + if definition.get('discriminator') != {}: + union_types.append(definition) def find_union(name): global union_types -- cgit v1.1 From ab916faddd16f0165e9cc2551f90699be8efde53 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:13 -0600 Subject: qapi: Use 'alternate' to replace anonymous union Previous patches have led up to the point where I create the new meta-type "'alternate':'Foo'". See the previous patches for documentation; I intentionally split as much work into earlier patches to minimize the size of this patch, but a lot of it is churn due to testsuite fallout after updating to the new type. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 0b88325..05c38c5 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -281,7 +281,6 @@ def check_union(expr, expr_info): "along with base" %name) # Two types of unions, determined by discriminator. - assert discriminator != {} # With no discriminator it is a simple union. if discriminator is None: @@ -344,17 +343,14 @@ def check_union(expr, expr_info): values[c_key] = key def check_alternate(expr, expr_info): - name = expr['union'] - base = expr.get('base') - discriminator = expr.get('discriminator') + name = expr['alternate'] members = expr['data'] values = { 'MAX': '(automatic)' } types_seen = {} - assert discriminator == {} - if base is not None: + if expr.get('base') is not None: raise QAPIExprError(expr_info, - "Anonymous union '%s' must not have a base" + "Alternate '%s' must not have a base" % name) # Check every branch @@ -363,23 +359,23 @@ def check_alternate(expr, expr_info): c_key = _generate_enum_string(key) if c_key in values: raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' clashes " - "with '%s'" % (name, key, values[c_key])) + "Alternate '%s' member '%s' clashes with '%s'" + % (name, key, values[c_key])) values[c_key] = key # Ensure alternates have no type conflicts. if isinstance(value, list): raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' must " + "Alternate '%s' member '%s' must " "not be array type" % (name, key)) qtype = find_alternate_member_qtype(value) if not qtype: raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' has " + "Alternate '%s' member '%s' has " "invalid type '%s'" % (name, key, value)) if qtype in types_seen: raise QAPIExprError(expr_info, - "Anonymous union '%s' member '%s' can't " + "Alternate '%s' member '%s' can't " "be distinguished from member '%s'" % (name, key, types_seen[qtype])) types_seen[qtype] = key @@ -412,10 +408,9 @@ def check_exprs(schema): if expr.has_key('enum'): check_enum(expr, info) elif expr.has_key('union'): - if expr.get('discriminator') == {}: - check_alternate(expr, info) - else: - check_union(expr, info) + check_union(expr, info) + elif expr.has_key('alternate'): + check_alternate(expr, info) elif expr.has_key('event'): check_event(expr, info) @@ -447,6 +442,8 @@ def parse_schema(input_file): if expr.has_key('union'): if not discriminator_find_enum_define(expr): add_enum('%sKind' % expr['union']) + elif expr.has_key('alternate'): + add_enum('%sKind' % expr['alternate']) # Final pass - validate that exprs make sense check_exprs(schema) @@ -557,8 +554,7 @@ def find_struct(name): def add_union(definition): global union_types - if definition.get('discriminator') != {}: - union_types.append(definition) + union_types.append(definition) def find_union(name): global union_types -- cgit v1.1 From 0545f6b8874c28d97369f2c83e5077e0461d4f12 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:15 -0600 Subject: qapi: Better error messages for bad expressions The previous commit demonstrated that the generator overlooked some fairly basic broken expressions: - missing metataype - metatype key has a non-string value - unknown key in relation to the metatype - conflicting metatype (this patch treats the second metatype as an unknown key of the first key visited, which is not necessarily the first key the user typed) Add check_keys to cover these situations, and update testcases to match. A couple other tests (enum-missing-data, indented-expr) had to change since the validation added here occurs so early. Conversely, changes to ident-with-escape results show that we still have problems where our handling of escape sequences differs from true JSON, which will matter down the road if we allow arbitrary default string values for optional parameters (but for now is not too bad, as we currently can avoid unicode escaping as we don't need to represent anything beyond C identifier material). 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 | 44 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 05c38c5..868f08b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -348,11 +348,6 @@ def check_alternate(expr, expr_info): values = { 'MAX': '(automatic)' } types_seen = {} - if expr.get('base') is not None: - raise QAPIExprError(expr_info, - "Alternate '%s' must not have a base" - % name) - # Check every branch for (key, value) in members.items(): # Check for conflicts in the generated enum @@ -414,6 +409,26 @@ def check_exprs(schema): elif expr.has_key('event'): check_event(expr, info) +def check_keys(expr_elem, meta, required, optional=[]): + expr = expr_elem['expr'] + info = expr_elem['info'] + name = expr[meta] + if not isinstance(name, str): + raise QAPIExprError(info, + "'%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: + raise QAPIExprError(info, + "Unknown key '%s' in %s '%s'" + % (key, meta, name)) + for key in required: + if not expr.has_key(key): + raise QAPIExprError(info, + "Key '%s' is missing from %s '%s'" + % (key, meta, name)) + + def parse_schema(input_file): # First pass: read entire file into memory try: @@ -425,15 +440,30 @@ def parse_schema(input_file): exprs = [] try: - # Next pass: learn the types. + # Next pass: learn the types and check for valid expression keys. At + # this point, top-level 'include' has already been flattened. for expr_elem in schema.exprs: expr = expr_elem['expr'] if expr.has_key('enum'): - add_enum(expr['enum'], expr.get('data')) + check_keys(expr_elem, 'enum', ['data']) + add_enum(expr['enum'], expr['data']) elif expr.has_key('union'): + check_keys(expr_elem, 'union', ['data'], + ['base', 'discriminator']) add_union(expr) + elif expr.has_key('alternate'): + check_keys(expr_elem, 'alternate', ['data']) elif expr.has_key('type'): + check_keys(expr_elem, 'type', ['data'], ['base']) add_struct(expr) + elif expr.has_key('command'): + check_keys(expr_elem, 'command', [], + ['data', 'returns', 'gen', 'success-response']) + elif expr.has_key('event'): + check_keys(expr_elem, 'event', [], ['data']) + else: + raise QAPIExprError(expr_elem['info'], + "Expression is missing metatype") exprs.append(expr) # Try again for hidden UnionKind enum -- cgit v1.1 From 4dc2e6906e1084fdd37bf67385c5dcd2c72ae22b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:17 -0600 Subject: 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 ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 14 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 868f08b..6a339d6 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) -- cgit v1.1 From e53188ada516c814a729551be2448684d6d8ce08 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Mon, 4 May 2015 09:05:18 -0600 Subject: qapi: Allow true, false and null in schema json In the near term, we will use it for a sensible-looking 'gen':false inside command declarations, instead of the current ugly 'gen':'no'. In the long term, it will allow conversion from shorthand with defaults mentioned only in side-band documentation: 'data':{'*flag':'bool', '*string':'str'} into an explicit default value documentation, as in: 'data':{'flag':{'type':'bool', 'optional':true, 'default':true}, 'string':{'type':'str', 'optional':true, 'default':null}} We still don't parse integer values (also necessary before we can allow explicit defaults), but that can come in a later series. Update the testsuite to match an improved error message. Signed-off-by: Fam Zheng Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 6a339d6..1dd91ee 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -158,6 +158,20 @@ class QAPISchema: return else: string += ch + elif self.tok in "tfn": + val = self.src[self.cursor - 1:] + if val.startswith("true"): + self.val = True + self.cursor += 3 + return + elif val.startswith("false"): + self.val = False + self.cursor += 4 + return + elif val.startswith("null"): + self.val = None + self.cursor += 3 + return elif self.tok == '\n': if self.cursor == len(self.src): self.tok = None @@ -197,8 +211,9 @@ class QAPISchema: if self.tok == ']': self.accept() return expr - if not self.tok in [ '{', '[', "'" ]: - raise QAPISchemaError(self, 'Expected "{", "[", "]" or string') + if not self.tok in "{['tfn": + raise QAPISchemaError(self, 'Expected "{", "[", "]", string, ' + 'boolean or "null"') while True: expr.append(self.get_expr(True)) if self.tok == ']': @@ -217,7 +232,7 @@ class QAPISchema: elif self.tok == '[': self.accept() expr = self.get_values() - elif self.tok == "'": + elif self.tok in "'tfn": expr = self.val self.accept() else: -- cgit v1.1 From dd883c6f0547f02ae805d02852ff3691f6d08f85 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:21 -0600 Subject: qapi: More rigourous checking of types Now that we know every expression is valid with regards to its keys, we can add further tests that those keys refer to valid types. With this patch, all uses of a type (the 'data': of command, type, union, alternate, and event; the 'returns': of command; the 'base': of type and union) must resolve to an appropriate subset of metatypes declared by the current qapi parse; this includes recursing into each member of a data dictionary. Dealing with '**' and nested anonymous structs will be done in later patches. Update the testsuite to match improved output. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 87 insertions(+), 9 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 1dd91ee..3c33e4e 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -276,6 +276,64 @@ def discriminator_find_enum_define(expr): return find_enum(discriminator_type) +def check_type(expr_info, source, value, allow_array = False, + allow_dict = False, allow_metas = []): + global all_names + orig_value = value + + if value is None: + return + + if value == '**': + return + + # Check if array type for value is okay + if isinstance(value, list): + if not allow_array: + raise QAPIExprError(expr_info, + "%s cannot be an array" % source) + if len(value) != 1 or not isinstance(value[0], str): + raise QAPIExprError(expr_info, + "%s: array type must contain single type name" + % source) + value = value[0] + orig_value = "array of %s" %value + + # Check if type name for value is okay + if isinstance(value, str): + if not value in all_names: + raise QAPIExprError(expr_info, + "%s uses unknown type '%s'" + % (source, orig_value)) + if not all_names[value] in allow_metas: + raise QAPIExprError(expr_info, + "%s cannot use %s type '%s'" + % (source, all_names[value], orig_value)) + return + + # value is a dictionary, check that each member is okay + if not isinstance(value, OrderedDict): + raise QAPIExprError(expr_info, + "%s should be a dictionary" % source) + if not allow_dict: + raise QAPIExprError(expr_info, + "%s should be a type name" % source) + for (key, arg) in value.items(): + check_type(expr_info, "Member '%s' of %s" % (key, source), arg, + allow_array=True, allow_dict=True, + allow_metas=['built-in', 'union', 'alternate', 'struct', + 'enum']) + +def check_command(expr, expr_info): + name = expr['command'] + check_type(expr_info, "'data' for command '%s'" % name, + expr.get('data'), allow_dict=True, + allow_metas=['union', 'struct']) + check_type(expr_info, "'returns' for command '%s'" % name, + expr.get('returns'), allow_array=True, allow_dict=True, + allow_metas=['built-in', 'union', 'alternate', 'struct', + 'enum']) + def check_event(expr, expr_info): global events name = expr['event'] @@ -284,7 +342,9 @@ def check_event(expr, expr_info): if name.upper() == 'MAX': raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created") events.append(name) - + check_type(expr_info, "'data' for event '%s'" % name, + expr.get('data'), allow_dict=True, + allow_metas=['union', 'struct']) if params: for argname, argentry, optional, structured in parse_args(params): if structured: @@ -313,6 +373,7 @@ def check_union(expr, expr_info): # With no discriminator it is a simple union. if discriminator is None: enum_define = None + allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum'] if base is not None: raise QAPIExprError(expr_info, "Simple union '%s' must not have a base" @@ -344,6 +405,7 @@ def check_union(expr, expr_info): "type '%s'" % (discriminator, base)) enum_define = find_enum(discriminator_type) + allow_metas=['struct'] # Do not allow string discriminator if not enum_define: raise QAPIExprError(expr_info, @@ -352,6 +414,11 @@ def check_union(expr, expr_info): # Check every branch for (key, value) in members.items(): + # Each value must name a known type; furthermore, in flat unions, + # branches must be a struct + check_type(expr_info, "Member '%s' of union '%s'" % (key, name), + value, allow_array=True, allow_metas=allow_metas) + # If the discriminator names an enum type, then all members # of 'data' must also be members of the enum type. if enum_define: @@ -387,15 +454,11 @@ def check_alternate(expr, expr_info): values[c_key] = key # Ensure alternates have no type conflicts. - if isinstance(value, list): - raise QAPIExprError(expr_info, - "Alternate '%s' member '%s' must " - "not be array type" % (name, key)) + check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name), + value, + allow_metas=['built-in', 'union', 'struct', 'enum']) qtype = find_alternate_member_qtype(value) - if not qtype: - raise QAPIExprError(expr_info, - "Alternate '%s' member '%s' has " - "invalid type '%s'" % (name, key, value)) + assert qtype if qtype in types_seen: raise QAPIExprError(expr_info, "Alternate '%s' member '%s' can't " @@ -423,6 +486,15 @@ def check_enum(expr, expr_info): % (name, member, values[key])) values[key] = member +def check_struct(expr, expr_info): + name = expr['type'] + members = expr['data'] + + check_type(expr_info, "'data' for type '%s'" % name, members, + allow_dict=True) + check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'), + allow_metas=['struct']) + def check_exprs(schema): for expr_elem in schema.exprs: expr = expr_elem['expr'] @@ -434,8 +506,14 @@ def check_exprs(schema): check_union(expr, info) elif expr.has_key('alternate'): check_alternate(expr, info) + elif expr.has_key('type'): + check_struct(expr, info) + elif expr.has_key('command'): + check_command(expr, info) elif expr.has_key('event'): check_event(expr, info) + else: + assert False, 'unexpected meta type' def check_keys(expr_elem, meta, required, optional=[]): expr = expr_elem['expr'] -- cgit v1.1 From c9e0a798691d8c45747b082206e789c8f50523c9 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:22 -0600 Subject: qapi: Require valid names Previous commits demonstrated that the generator overlooked various bad naming situations: - types, commands, and events need a valid name - enum members must be valid names, when combined with prefix - union and alternate branches cannot be marked optional Valid upstream names match [a-zA-Z][a-zA-Z0-9_-]*; valid downstream names match __[a-zA-Z][a-zA-Z0-9._-]*. Enumerations match the weaker [a-zA-Z0-9._-]+ (in part thanks to QKeyCode picking an enum that starts with a digit, which we can't change now due to backwards compatibility). Rather than call out three separate regex, this patch just uses a broader combination that allows both upstream and downstream names, as well as a small hack that realizes that any enum name is merely a suffix to an already valid name prefix (that is, any enum name is valid if prepending _ fits the normal rules). We could reject new enumeration names beginning with a digit by whitelisting existing exceptions. We could also be stricter about the distinction between upstream names (no leading underscore, no use of dot) and downstream (mandatory leading double underscore), but it is probably not worth the bother. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 63 +++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 17 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 3c33e4e..5bc32e3 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -276,8 +276,31 @@ def discriminator_find_enum_define(expr): return find_enum(discriminator_type) +valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$') +def check_name(expr_info, source, name, allow_optional = False, + enum_member = False): + global valid_name + membername = name + + if not isinstance(name, str): + raise QAPIExprError(expr_info, + "%s requires a string name" % source) + if name.startswith('*'): + membername = name[1:] + if not allow_optional: + raise QAPIExprError(expr_info, + "%s does not allow optional name '%s'" + % (source, name)) + # Enum members can start with a digit, because the generated C + # code always prefixes it with the enum name + if enum_member: + membername = '_' + membername + if not valid_name.match(membername): + raise QAPIExprError(expr_info, + "%s uses invalid name '%s'" % (source, name)) + def check_type(expr_info, source, value, allow_array = False, - allow_dict = False, allow_metas = []): + allow_dict = False, allow_optional = False, allow_metas = []): global all_names orig_value = value @@ -319,18 +342,21 @@ def check_type(expr_info, source, value, allow_array = False, raise QAPIExprError(expr_info, "%s should be a type name" % source) for (key, arg) in value.items(): + check_name(expr_info, "Member of %s" % source, key, + allow_optional=allow_optional) check_type(expr_info, "Member '%s' of %s" % (key, source), arg, - allow_array=True, allow_dict=True, + allow_array=True, allow_dict=True, allow_optional=True, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) def check_command(expr, expr_info): name = expr['command'] check_type(expr_info, "'data' for command '%s'" % name, - expr.get('data'), allow_dict=True, + expr.get('data'), allow_dict=True, allow_optional=True, allow_metas=['union', 'struct']) check_type(expr_info, "'returns' for command '%s'" % name, expr.get('returns'), allow_array=True, allow_dict=True, + allow_optional=True, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) @@ -343,7 +369,7 @@ def check_event(expr, expr_info): raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created") events.append(name) check_type(expr_info, "'data' for event '%s'" % name, - expr.get('data'), allow_dict=True, + expr.get('data'), allow_dict=True, allow_optional=True, allow_metas=['union', 'struct']) if params: for argname, argentry, optional, structured in parse_args(params): @@ -392,12 +418,10 @@ def check_union(expr, expr_info): "Base '%s' is not a valid type" % base) - # The value of member 'discriminator' must name a member of the - # base type. - if not isinstance(discriminator, str): - raise QAPIExprError(expr_info, - "Flat union '%s' discriminator must be a string" - % name) + # The value of member 'discriminator' must name a non-optional + # member of the base type. + check_name(expr_info, "Discriminator of flat union '%s'" % name, + discriminator) discriminator_type = base_fields.get(discriminator) if not discriminator_type: raise QAPIExprError(expr_info, @@ -414,6 +438,8 @@ def check_union(expr, expr_info): # Check every branch for (key, value) in members.items(): + check_name(expr_info, "Member of union '%s'" % name, key) + # Each value must name a known type; furthermore, in flat unions, # branches must be a struct check_type(expr_info, "Member '%s' of union '%s'" % (key, name), @@ -445,6 +471,8 @@ def check_alternate(expr, expr_info): # Check every branch for (key, value) in members.items(): + check_name(expr_info, "Member of alternate '%s'" % name, key) + # Check for conflicts in the generated enum c_key = _generate_enum_string(key) if c_key in values: @@ -475,10 +503,8 @@ def check_enum(expr, expr_info): raise QAPIExprError(expr_info, "Enum '%s' requires an array for 'data'" % name) for member in members: - if not isinstance(member, str): - raise QAPIExprError(expr_info, - "Enum '%s' member '%s' is not a string" - % (name, member)) + check_name(expr_info, "Member of enum '%s'" %name, member, + enum_member=True) key = _generate_enum_string(member) if key in values: raise QAPIExprError(expr_info, @@ -491,7 +517,7 @@ def check_struct(expr, expr_info): members = expr['data'] check_type(expr_info, "'data' for type '%s'" % name, members, - allow_dict=True) + allow_dict=True, allow_optional=True) check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'), allow_metas=['struct']) @@ -682,8 +708,11 @@ def type_name(name): return c_list_type(name[0]) return name -def add_name(name, info, meta, implicit = False): +def add_name(name, info, meta, implicit = False, source = None): global all_names + if not source: + source = "'%s'" % meta + check_name(info, source, name) if name in all_names: raise QAPIExprError(info, "%s '%s' is already defined" @@ -697,7 +726,7 @@ def add_name(name, info, meta, implicit = False): def add_struct(definition, info): global struct_types name = definition['type'] - add_name(name, info, 'struct') + add_name(name, info, 'struct', source="'type'") struct_types.append(definition) def find_struct(name): -- cgit v1.1 From 10d4d997f86cf2a4ce89145df5658952d5722e56 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:23 -0600 Subject: qapi: Whitelist commands that don't return dictionary ...or an array of dictionaries. Although we have to cater to existing commands, returning a non-dictionary means the command is not extensible (no new name/value pairs can be added if more information must be returned in parallel). By making the whitelist explicit, any new command that falls foul of this practice will have to be self-documenting, which will encourage developers to either justify the action or rework the design to use a dictionary after all. It's a little bit sloppy that we share a single whitelist among three clients (it's too permissive for each). If this is a problem, a future patch could tighten things by having the generator take the whitelist as an argument (as in scripts/qapi-commands.py --legacy-returns=...), or by having the generator output C code that requires explicit use of the whitelist (as in: #ifndef FROBNICATE_LEGACY_RETURN_OK # error Command 'frobnicate' should return a dictionary #endif then having the callers define appropriate macros). But until we need such fine-grained separation (if ever), this patch does the job just fine. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 5bc32e3..2402d05 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -32,6 +32,30 @@ builtin_types = { 'size': 'QTYPE_QINT', } +# Whitelist of commands allowed to return a non-dictionary +returns_whitelist = [ + # From QMP: + 'human-monitor-command', + 'query-migrate-cache-size', + 'query-tpm-models', + 'query-tpm-types', + 'ringbuf-read', + + # From QGA: + 'guest-file-open', + 'guest-fsfreeze-freeze', + 'guest-fsfreeze-freeze-list', + 'guest-fsfreeze-status', + 'guest-fsfreeze-thaw', + 'guest-get-time', + 'guest-set-vcpus', + 'guest-sync', + 'guest-sync-delimited', + + # From qapi-schema-test: + 'user_def_cmd3', +] + enum_types = [] struct_types = [] union_types = [] @@ -354,11 +378,12 @@ def check_command(expr, expr_info): check_type(expr_info, "'data' for command '%s'" % name, expr.get('data'), allow_dict=True, allow_optional=True, allow_metas=['union', 'struct']) + returns_meta = ['union', 'struct'] + if name in returns_whitelist: + returns_meta += ['built-in', 'alternate', 'enum'] check_type(expr_info, "'returns' for command '%s'" % name, expr.get('returns'), allow_array=True, allow_dict=True, - allow_optional=True, - allow_metas=['built-in', 'union', 'alternate', 'struct', - 'enum']) + allow_optional=True, allow_metas=returns_meta) def check_event(expr, expr_info): global events -- cgit v1.1 From 2cbf09925ad45401673a79ab77f67de2f04a826c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:24 -0600 Subject: qapi: More rigorous checking for type safety bypass Now that we have a way to validate every type, we can also be stricter about enforcing that callers that want to bypass type safety in generated code. Prior to this patch, it didn't matter what value was associated with the key 'gen', but it looked odd that 'gen':'yes' could result in bypassing the generated code. These changes also enforce the changes made earlier in the series for documentation and consolidation of using '**' as the wildcard type, as well as 'gen':false as the canonical spelling for requesting type bypass. Note that 'gen':false is a one-way switch away from the default; we do not support 'gen':true (similar for 'success-response'). In practice, this doesn't matter. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 2402d05..e391b5a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -324,14 +324,15 @@ def check_name(expr_info, source, name, allow_optional = False, "%s uses invalid name '%s'" % (source, name)) def check_type(expr_info, source, value, allow_array = False, - allow_dict = False, allow_optional = False, allow_metas = []): + allow_dict = False, allow_optional = False, + allow_star = False, allow_metas = []): global all_names orig_value = value if value is None: return - if value == '**': + if allow_star and value == '**': return # Check if array type for value is okay @@ -348,6 +349,10 @@ def check_type(expr_info, source, value, allow_array = False, # Check if type name for value is okay if isinstance(value, str): + if value == '**': + raise QAPIExprError(expr_info, + "%s uses '**' but did not request 'gen':false" + % source) if not value in all_names: raise QAPIExprError(expr_info, "%s uses unknown type '%s'" @@ -371,19 +376,22 @@ def check_type(expr_info, source, value, allow_array = False, check_type(expr_info, "Member '%s' of %s" % (key, source), arg, allow_array=True, allow_dict=True, allow_optional=True, allow_metas=['built-in', 'union', 'alternate', 'struct', - 'enum']) + 'enum'], allow_star=allow_star) def check_command(expr, expr_info): name = expr['command'] + allow_star = expr.has_key('gen') + check_type(expr_info, "'data' for command '%s'" % name, expr.get('data'), allow_dict=True, allow_optional=True, - allow_metas=['union', 'struct']) + allow_metas=['union', 'struct'], allow_star=allow_star) returns_meta = ['union', 'struct'] if name in returns_whitelist: returns_meta += ['built-in', 'alternate', 'enum'] check_type(expr_info, "'returns' for command '%s'" % name, expr.get('returns'), allow_array=True, allow_dict=True, - allow_optional=True, allow_metas=returns_meta) + allow_optional=True, allow_metas=returns_meta, + allow_star=allow_star) def check_event(expr, expr_info): global events @@ -579,6 +587,10 @@ def check_keys(expr_elem, meta, required, optional=[]): raise QAPIExprError(info, "Unknown key '%s' in %s '%s'" % (key, meta, name)) + if (key == 'gen' or key == 'success-response') and value != False: + raise QAPIExprError(info, + "'%s' of %s '%s' should only use false value" + % (key, meta, name)) for key in required: if not expr.has_key(key): raise QAPIExprError(info, -- cgit v1.1 From fd41dd4eae5f7ea92f10c04cb3f217727fcee91f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:25 -0600 Subject: qapi: Prefer 'struct' over 'type' in generator Referring to "type" as both a meta-type (built-in, enum, union, alternate, or struct) and a specific type (the name that the schema uses for declaring structs) is confusing. The confusion is only made worse by the fact that the generator mostly already refers to struct even when dealing with expr['type']. This commit changes the generator to consistently refer to it as struct everywhere, plus a single back-compat tweak that allows accepting the existing .json files as-is, so that the meat of this change is separate from the mindless churn of that change. Fix the testsuite fallout for error messages that change, and in some cases, become more legible. Improve comments to better match our intentions where a struct (rather than any complex type) is required. Note that in some cases, an error message now refers to 'struct' while the schema still refers to 'type'; that will be cleaned up in the later commit to the schema. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index e391b5a..e50fec8 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -419,7 +419,7 @@ def check_union(expr, expr_info): members = expr['data'] values = { 'MAX': '(automatic)' } - # If the object has a member 'base', its value must name a complex type, + # If the object has a member 'base', its value must name a struct, # and there must be a discriminator. if base is not None: if discriminator is None: @@ -448,18 +448,18 @@ def check_union(expr, expr_info): base_fields = find_base_fields(base) if not base_fields: raise QAPIExprError(expr_info, - "Base '%s' is not a valid type" + "Base '%s' is not a valid struct" % base) # The value of member 'discriminator' must name a non-optional - # member of the base type. + # member of the base struct. check_name(expr_info, "Discriminator of flat union '%s'" % name, discriminator) discriminator_type = base_fields.get(discriminator) if not discriminator_type: raise QAPIExprError(expr_info, "Discriminator '%s' is not a member of base " - "type '%s'" + "struct '%s'" % (discriminator, base)) enum_define = find_enum(discriminator_type) allow_metas=['struct'] @@ -546,12 +546,12 @@ def check_enum(expr, expr_info): values[key] = member def check_struct(expr, expr_info): - name = expr['type'] + name = expr['struct'] members = expr['data'] - check_type(expr_info, "'data' for type '%s'" % name, members, + check_type(expr_info, "'data' for struct '%s'" % name, members, allow_dict=True, allow_optional=True) - check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'), + check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'), allow_metas=['struct']) def check_exprs(schema): @@ -565,7 +565,7 @@ def check_exprs(schema): check_union(expr, info) elif expr.has_key('alternate'): check_alternate(expr, info) - elif expr.has_key('type'): + elif expr.has_key('struct'): check_struct(expr, info) elif expr.has_key('command'): check_command(expr, info) @@ -617,6 +617,20 @@ def parse_schema(input_file): for expr_elem in schema.exprs: expr = expr_elem['expr'] info = expr_elem['info'] + + # back-compat hack until all schemas have been converted; + # preserve the ordering of the original expression + if expr.has_key('type'): + seen_type = False + for (key, value) in expr.items(): + if key == 'type': + seen_type = True + del expr['type'] + expr['struct'] = value + elif seen_type: + del expr[key] + expr[key] = value + if expr.has_key('enum'): check_keys(expr_elem, 'enum', ['data']) add_enum(expr['enum'], info, expr['data']) @@ -627,8 +641,8 @@ def parse_schema(input_file): 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']) + elif expr.has_key('struct'): + check_keys(expr_elem, 'struct', ['data'], ['base']) add_struct(expr, info) elif expr.has_key('command'): check_keys(expr_elem, 'command', [], @@ -745,11 +759,9 @@ def type_name(name): return c_list_type(name[0]) return name -def add_name(name, info, meta, implicit = False, source = None): +def add_name(name, info, meta, implicit = False): global all_names - if not source: - source = "'%s'" % meta - check_name(info, source, name) + check_name(info, "'%s'" % meta, name) if name in all_names: raise QAPIExprError(info, "%s '%s' is already defined" @@ -762,14 +774,14 @@ def add_name(name, info, meta, implicit = False, source = None): def add_struct(definition, info): global struct_types - name = definition['type'] - add_name(name, info, 'struct', source="'type'") + name = definition['struct'] + add_name(name, info, 'struct') struct_types.append(definition) def find_struct(name): global struct_types for struct in struct_types: - if struct['type'] == name: + if struct['struct'] == name: return struct return None -- cgit v1.1 From 3e391d355644b2bff7c9f187759aadb46c6e051f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:28 -0600 Subject: qapi: Forbid 'type' in schema Referring to "type" as both a meta-type (built-in, enum, union, alternate, or struct) and a specific type (the name that the schema uses for declaring structs) is confusing. Finish up the conversion to using "struct" in qapi schema by removing the hack in the generator that allowed 'type'. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 14 -------------- 1 file changed, 14 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index e50fec8..333f59a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -617,20 +617,6 @@ def parse_schema(input_file): for expr_elem in schema.exprs: expr = expr_elem['expr'] info = expr_elem['info'] - - # back-compat hack until all schemas have been converted; - # preserve the ordering of the original expression - if expr.has_key('type'): - seen_type = False - for (key, value) in expr.items(): - if key == 'type': - seen_type = True - del expr['type'] - expr['struct'] = value - elif seen_type: - del expr[key] - expr[key] = value - if expr.has_key('enum'): check_keys(expr_elem, 'enum', ['data']) add_enum(expr['enum'], info, expr['data']) -- cgit v1.1 From 6b5abc7df7ef9aadb3ff0eba6ccf4f1f0181e2e1 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:33 -0600 Subject: qapi: Drop support for inline nested types A future patch will be using a 'name':{dictionary} entry in the QAPI schema to specify a default value for an optional argument (see previous commit messages for more details why); but existing use of inline nested structs conflicts with that goal. Now that all commands have been changed to avoid inline nested structs, nuke support for them, and turn it into a hard error. Update the testsuite to reflect tighter parsing rules. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 333f59a..44898b0 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -373,10 +373,12 @@ def check_type(expr_info, source, value, allow_array = False, for (key, arg) in value.items(): check_name(expr_info, "Member of %s" % source, key, allow_optional=allow_optional) + # Todo: allow dictionaries to represent default values of + # an optional argument. check_type(expr_info, "Member '%s' of %s" % (key, source), arg, - allow_array=True, allow_dict=True, allow_optional=True, + allow_array=True, allow_star=allow_star, allow_metas=['built-in', 'union', 'alternate', 'struct', - 'enum'], allow_star=allow_star) + 'enum']) def check_command(expr, expr_info): name = expr['command'] @@ -404,13 +406,6 @@ def check_event(expr, expr_info): check_type(expr_info, "'data' for event '%s'" % name, expr.get('data'), allow_dict=True, allow_optional=True, allow_metas=['union', 'struct']) - if params: - for argname, argentry, optional, structured in parse_args(params): - if structured: - raise QAPIExprError(expr_info, - "Nested structure define in event is not " - "supported, event '%s', argname '%s'" - % (expr['event'], argname)) def check_union(expr, expr_info): name = expr['union'] @@ -671,13 +666,12 @@ def parse_args(typeinfo): argname = member argentry = typeinfo[member] optional = False - structured = False if member.startswith('*'): argname = member[1:] optional = True - if isinstance(argentry, OrderedDict): - structured = True - yield (argname, argentry, optional, structured) + # Todo: allow argentry to be OrderedDict, for providing the + # value of an optional argument. + yield (argname, argentry, optional) def de_camel_case(name): new_name = '' -- cgit v1.1 From a7f5966b297330f6492020019544ae87c45d699b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:36 -0600 Subject: qapi: Support (subset of) \u escapes in strings The handling of \ inside QAPI strings was less than ideal, and really only worked JSON's \/, \\, \", and our extension of \' (an obvious extension, when you realize we use '' instead of "" for strings). For other things, like '\n', it resulted in a literal 'n' instead of a newline. Of course, at the moment, we really have no use for escaped characters, as QAPI has to map to C identifiers, and we currently support ASCII only for that. But down the road, we may add support for default values for string parameters to a command or struct; if that happens, it would be nice to correctly support all JSON escape sequences, such as \n or \uXXXX. This gets us closer, by supporting Unicode escapes in the ASCII range. Since JSON does not require \OCTAL or \xXX escapes, and our QMP implementation does not understand them either, I intentionally reject it here, but it would be an easy addition if we desired it. Likewise, intentionally refusing the NUL byte means we don't have to worry about C strings being shorter than the qapi input. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 44898b0..6a9aa24 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -173,7 +173,41 @@ class QAPISchema: raise QAPISchemaError(self, 'Missing terminating "\'"') if esc: - string += ch + if ch == 'b': + string += '\b' + elif ch == 'f': + string += '\f' + elif ch == 'n': + string += '\n' + elif ch == 'r': + string += '\r' + elif ch == 't': + string += '\t' + elif ch == 'u': + value = 0 + for x in range(0, 4): + ch = self.src[self.cursor] + self.cursor += 1 + if ch not in "0123456789abcdefABCDEF": + raise QAPISchemaError(self, + '\\u escape needs 4 ' + 'hex digits') + value = (value << 4) + int(ch, 16) + # If Python 2 and 3 didn't disagree so much on + # how to handle Unicode, then we could allow + # Unicode string defaults. But most of QAPI is + # ASCII-only, so we aren't losing much for now. + if not value or value > 0x7f: + raise QAPISchemaError(self, + 'For now, \\u escape ' + 'only supports non-zero ' + 'values up to \\u007f') + string += chr(value) + elif ch in "\\/'\"": + string += ch + else: + raise QAPISchemaError(self, + "Unknown escape \\%s" %ch) esc = False elif ch == "\\": esc = True -- cgit v1.1 From ff55d72eaf9628e7d58e7b067b361cdbf789c9f4 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:37 -0600 Subject: qapi: Check for member name conflicts with a base class Our type inheritance for both 'struct' and for flat 'union' merges key/value pairs from the base class with those from the type in question. Although the C code currently boxes things so that there is a distinction between which member is referred to, the QMP wire format does not allow passing a key more than once in a single object. Besides, if we ever change the generated C code to not be quite so boxy, we'd want to avoid duplicate member names there, too. Fix a testsuite entry added in an earlier patch, as well as adding a couple more tests to ensure we have appropriate coverage. Ensure that collisions are detected, regardless of whether there is a difference in opinion on whether the member name is optional. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 6a9aa24..166b74f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -414,6 +414,20 @@ def check_type(expr_info, source, value, allow_array = False, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) +def check_member_clash(expr_info, base_name, data, source = ""): + base = find_struct(base_name) + assert base + base_members = base['data'] + for key in data.keys(): + if key.startswith('*'): + key = key[1:] + if key in base_members or "*" + key in base_members: + raise QAPIExprError(expr_info, + "Member name '%s'%s clashes with base '%s'" + % (key, source, base_name)) + if base.get('base'): + check_member_clash(expr_info, base['base'], data, source) + def check_command(expr, expr_info): name = expr['command'] allow_star = expr.has_key('gen') @@ -503,9 +517,14 @@ def check_union(expr, expr_info): check_name(expr_info, "Member of union '%s'" % name, key) # Each value must name a known type; furthermore, in flat unions, - # branches must be a struct + # branches must be a struct with no overlapping member names check_type(expr_info, "Member '%s' of union '%s'" % (key, name), value, allow_array=True, allow_metas=allow_metas) + if base: + branch_struct = find_struct(value) + assert branch_struct + check_member_clash(expr_info, base, branch_struct['data'], + " of branch '%s'" % key) # If the discriminator names an enum type, then all members # of 'data' must also be members of the enum type. @@ -582,6 +601,8 @@ def check_struct(expr, expr_info): allow_dict=True, allow_optional=True) check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'), allow_metas=['struct']) + if expr.get('base'): + check_member_clash(expr_info, expr['base'], expr['data']) def check_exprs(schema): for expr_elem in schema.exprs: -- cgit v1.1