From patchwork Wed Apr 29 13:06:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 466019 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7BC651402BD for ; Wed, 29 Apr 2015 23:19:47 +1000 (AEST) Received: from localhost ([::1]:38887 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRtx-00018d-Cu for incoming@patchwork.ozlabs.org; Wed, 29 Apr 2015 09:19:45 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRht-0007rW-2I for qemu-devel@nongnu.org; Wed, 29 Apr 2015 09:07:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnRhp-000145-IK for qemu-devel@nongnu.org; Wed, 29 Apr 2015 09:07:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34229) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRhp-00012b-7A for qemu-devel@nongnu.org; Wed, 29 Apr 2015 09:07:13 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3TD7Bq6016650 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 29 Apr 2015 09:07:12 -0400 Received: from red.redhat.com (ovpn-113-25.phx2.redhat.com [10.3.113.25]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3TD6ww4012100; Wed, 29 Apr 2015 09:07:11 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Wed, 29 Apr 2015 07:06:40 -0600 Message-Id: <1430312814-19706-26-git-send-email-eblake@redhat.com> In-Reply-To: <1430312814-19706-1-git-send-email-eblake@redhat.com> References: <1430312814-19706-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, berto@igalia.com, armbru@redhat.com Subject: [Qemu-devel] [PATCH v7 25/39] qapi: Require valid names X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- v7: improve commit message --- scripts/qapi.py | 63 ++++++++++++++++------ tests/qapi-schema/bad-ident.err | 1 + tests/qapi-schema/bad-ident.exit | 2 +- tests/qapi-schema/bad-ident.json | 2 +- tests/qapi-schema/bad-ident.out | 3 -- tests/qapi-schema/enum-bad-name.err | 1 + tests/qapi-schema/enum-bad-name.exit | 2 +- tests/qapi-schema/enum-bad-name.json | 2 +- tests/qapi-schema/enum-bad-name.out | 3 -- tests/qapi-schema/enum-dict-member.err | 2 +- tests/qapi-schema/flat-union-bad-discriminator.err | 2 +- .../flat-union-optional-discriminator.err | 1 + .../flat-union-optional-discriminator.exit | 2 +- .../flat-union-optional-discriminator.json | 2 +- .../flat-union-optional-discriminator.out | 7 --- tests/qapi-schema/union-optional-branch.err | 1 + tests/qapi-schema/union-optional-branch.exit | 2 +- tests/qapi-schema/union-optional-branch.json | 2 +- tests/qapi-schema/union-optional-branch.out | 3 -- 19 files changed, 60 insertions(+), 43 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 61e1c43..c6ab8f3 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 = "_%s" %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): diff --git a/tests/qapi-schema/bad-ident.err b/tests/qapi-schema/bad-ident.err index e69de29..42b490c 100644 --- a/tests/qapi-schema/bad-ident.err +++ b/tests/qapi-schema/bad-ident.err @@ -0,0 +1 @@ +tests/qapi-schema/bad-ident.json:2: 'type' does not allow optional name '*oops' diff --git a/tests/qapi-schema/bad-ident.exit b/tests/qapi-schema/bad-ident.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/bad-ident.exit +++ b/tests/qapi-schema/bad-ident.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/bad-ident.json b/tests/qapi-schema/bad-ident.json index f139110..da949e8 100644 --- a/tests/qapi-schema/bad-ident.json +++ b/tests/qapi-schema/bad-ident.json @@ -1,2 +1,2 @@ -# FIXME: we should reject creating a type name with bad name +# we reject creating a type name with bad name { 'type': '*oops', 'data': { 'i': 'int' } } diff --git a/tests/qapi-schema/bad-ident.out b/tests/qapi-schema/bad-ident.out index 165e346..e69de29 100644 --- a/tests/qapi-schema/bad-ident.out +++ b/tests/qapi-schema/bad-ident.out @@ -1,3 +0,0 @@ -[OrderedDict([('type', '*oops'), ('data', OrderedDict([('i', 'int')]))])] -[] -[OrderedDict([('type', '*oops'), ('data', OrderedDict([('i', 'int')]))])] diff --git a/tests/qapi-schema/enum-bad-name.err b/tests/qapi-schema/enum-bad-name.err index e69de29..9c3c100 100644 --- a/tests/qapi-schema/enum-bad-name.err +++ b/tests/qapi-schema/enum-bad-name.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-bad-name.json:2: Member of enum 'MyEnum' uses invalid name 'not^possible' diff --git a/tests/qapi-schema/enum-bad-name.exit b/tests/qapi-schema/enum-bad-name.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-bad-name.exit +++ b/tests/qapi-schema/enum-bad-name.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-bad-name.json b/tests/qapi-schema/enum-bad-name.json index 0c32448..8506562 100644 --- a/tests/qapi-schema/enum-bad-name.json +++ b/tests/qapi-schema/enum-bad-name.json @@ -1,2 +1,2 @@ -# FIXME: we should ensure all enum names can map to C +# we ensure all enum names can map to C { 'enum': 'MyEnum', 'data': [ 'not^possible' ] } diff --git a/tests/qapi-schema/enum-bad-name.out b/tests/qapi-schema/enum-bad-name.out index d24ea49..e69de29 100644 --- a/tests/qapi-schema/enum-bad-name.out +++ b/tests/qapi-schema/enum-bad-name.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', ['not^possible'])])] -[{'enum_name': 'MyEnum', 'enum_values': ['not^possible']}] -[] diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err index 7e966a8..8ca146e 100644 --- a/tests/qapi-schema/enum-dict-member.err +++ b/tests/qapi-schema/enum-dict-member.err @@ -1 +1 @@ -tests/qapi-schema/enum-dict-member.json:2: Enum 'MyEnum' member 'OrderedDict([('value', 'str')])' is not a string +tests/qapi-schema/enum-dict-member.json:2: Member of enum 'MyEnum' requires a string name diff --git a/tests/qapi-schema/flat-union-bad-discriminator.err b/tests/qapi-schema/flat-union-bad-discriminator.err index 507e2ba..c38cc8e 100644 --- a/tests/qapi-schema/flat-union-bad-discriminator.err +++ b/tests/qapi-schema/flat-union-bad-discriminator.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-bad-discriminator.json:11: Flat union 'TestUnion' discriminator must be a string +tests/qapi-schema/flat-union-bad-discriminator.json:11: Discriminator of flat union 'TestUnion' requires a string name diff --git a/tests/qapi-schema/flat-union-optional-discriminator.err b/tests/qapi-schema/flat-union-optional-discriminator.err index e69de29..aaabedb 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.err +++ b/tests/qapi-schema/flat-union-optional-discriminator.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-optional-discriminator.json:6: Discriminator of flat union 'MyUnion' does not allow optional name '*switch' diff --git a/tests/qapi-schema/flat-union-optional-discriminator.exit b/tests/qapi-schema/flat-union-optional-discriminator.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.exit +++ b/tests/qapi-schema/flat-union-optional-discriminator.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/flat-union-optional-discriminator.json b/tests/qapi-schema/flat-union-optional-discriminator.json index ece0d31..25ce0e6 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.json +++ b/tests/qapi-schema/flat-union-optional-discriminator.json @@ -1,4 +1,4 @@ -# FIXME: we should require the discriminator to be non-optional +# we require the discriminator to be non-optional { 'enum': 'Enum', 'data': [ 'one', 'two' ] } { 'type': 'Base', 'data': { '*switch': 'Enum' } } diff --git a/tests/qapi-schema/flat-union-optional-discriminator.out b/tests/qapi-schema/flat-union-optional-discriminator.out index bb7db00..e69de29 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.out +++ b/tests/qapi-schema/flat-union-optional-discriminator.out @@ -1,7 +0,0 @@ -[OrderedDict([('enum', 'Enum'), ('data', ['one', 'two'])]), - OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))]), - OrderedDict([('type', 'Branch'), ('data', OrderedDict([('name', 'str')]))]), - OrderedDict([('union', 'MyUnion'), ('base', 'Base'), ('discriminator', '*switch'), ('data', OrderedDict([('one', 'Branch'), ('two', 'Branch')]))])] -[{'enum_name': 'Enum', 'enum_values': ['one', 'two']}] -[OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))]), - OrderedDict([('type', 'Branch'), ('data', OrderedDict([('name', 'str')]))])] diff --git a/tests/qapi-schema/union-optional-branch.err b/tests/qapi-schema/union-optional-branch.err index e69de29..3ada133 100644 --- a/tests/qapi-schema/union-optional-branch.err +++ b/tests/qapi-schema/union-optional-branch.err @@ -0,0 +1 @@ +tests/qapi-schema/union-optional-branch.json:2: Member of union 'Union' does not allow optional name '*a' diff --git a/tests/qapi-schema/union-optional-branch.exit b/tests/qapi-schema/union-optional-branch.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/union-optional-branch.exit +++ b/tests/qapi-schema/union-optional-branch.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/union-optional-branch.json b/tests/qapi-schema/union-optional-branch.json index c513db7..591615f 100644 --- a/tests/qapi-schema/union-optional-branch.json +++ b/tests/qapi-schema/union-optional-branch.json @@ -1,2 +1,2 @@ -# FIXME: union branches cannot be optional +# union branches cannot be optional { 'union': 'Union', 'data': { '*a': 'int', 'b': 'str' } } diff --git a/tests/qapi-schema/union-optional-branch.out b/tests/qapi-schema/union-optional-branch.out index b03b5d2..e69de29 100644 --- a/tests/qapi-schema/union-optional-branch.out +++ b/tests/qapi-schema/union-optional-branch.out @@ -1,3 +0,0 @@ -[OrderedDict([('union', 'Union'), ('data', OrderedDict([('*a', 'int'), ('b', 'str')]))])] -[{'enum_name': 'UnionKind', 'enum_values': None}] -[]