From patchwork Tue Mar 24 20:03:46 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 454027 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 D231614009B for ; Wed, 25 Mar 2015 07:19:31 +1100 (AEDT) Received: from localhost ([::1]:34321 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaVIP-0008Ec-72 for incoming@patchwork.ozlabs.org; Tue, 24 Mar 2015 16:19:29 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43753) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaV3o-0000bp-GR for qemu-devel@nongnu.org; Tue, 24 Mar 2015 16:04:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaV3g-00047u-HZ for qemu-devel@nongnu.org; Tue, 24 Mar 2015 16:04:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37466) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaV3g-00047n-7E for qemu-devel@nongnu.org; Tue, 24 Mar 2015 16:04:16 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id DA0228E917; Tue, 24 Mar 2015 20:04:15 +0000 (UTC) Received: from red.redhat.com (ovpn-113-74.phx2.redhat.com [10.3.113.74]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2OK3x7i028300; Tue, 24 Mar 2015 16:04:15 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Tue, 24 Mar 2015 14:03:46 -0600 Message-Id: <1427227433-5030-22-git-send-email-eblake@redhat.com> In-Reply-To: <1427227433-5030-1-git-send-email-eblake@redhat.com> References: <1427227433-5030-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, wenchaoqemu@gmail.com, lcapitulino@redhat.com Subject: [Qemu-devel] [PATCH v5 21/28] 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 - union and alternate branches cannot be marked optional The set of valid names includes [a-zA-Z0-9._-] (where '.' is useful only in downstream extensions). Signed-off-by: Eric Blake --- scripts/qapi.py | 57 ++++++++++++++++------ 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/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 | 5 -- 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 -- 14 files changed, 53 insertions(+), 32 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index c42683b..ed5385a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -15,6 +15,7 @@ import re from ordereddict import OrderedDict import os import sys +import string builtin_types = { 'str': 'QTYPE_QSTRING', @@ -276,8 +277,27 @@ def discriminator_find_enum_define(expr): return find_enum(discriminator_type) +valid_characters = set(string.ascii_letters + string.digits + '.' + '-' + '_') +def check_name(expr_info, source, name, allow_optional = False): + membername = name + + if not isinstance(name, str): + raise QAPIExprError(expr_info, + "%s requires a string name" % source) + if name == '**': + return + if name.startswith('*'): + membername = name[1:] + if not allow_optional: + raise QAPIExprError(expr_info, + "%s does not allow optional name '%s'" + % (source, name)) + if not set(membername) <= valid_characters: + raise QAPIExprError(expr_info, + "%s uses invalid name '%s'" % (source, name)) + def check_type(expr_info, source, data, allow_array = False, - allowed_metas = [], allow_dict = True): + allowed_metas = [], allow_dict = True, allow_optional = False): global all_names if data is None: @@ -317,21 +337,23 @@ def check_type(expr_info, source, data, allow_array = False, raise QAPIExprError(expr_info, "%s should be a type name" % source) for (key, value) in data.items(): + check_name(expr_info, "Member of %s" % source, key, + allow_optional=allow_optional) check_type(expr_info, "Member '%s' of %s" % (key, source), value, allow_array=True, allowed_metas=['built-in', 'union', 'alternate', 'struct', 'enum'], - allow_dict=True) + allow_dict=True, allow_optional=True) def check_command(expr, expr_info): name = expr['command'] check_type(expr_info, "'data' for command '%s'" % name, expr.get('data'), - allowed_metas=['union', 'struct']) + allowed_metas=['union', 'struct'], allow_optional=True) check_type(expr_info, "'returns' for command '%s'" % name, expr.get('returns'), allow_array=True, allowed_metas=['built-in', 'union', 'alternate', 'struct', - 'enum']) + 'enum'], allow_optional=True) def check_event(expr, expr_info): global events @@ -345,7 +367,8 @@ def check_event(expr, expr_info): % name) events.append(name) check_type(expr_info, "'data' for event '%s'" % name, - expr.get('data'), allowed_metas=['union', 'struct']) + expr.get('data'), allowed_metas=['union', 'struct'], + allow_optional=True) if params: for argname, argentry, optional, structured in parse_args(params): if structured: @@ -385,12 +408,10 @@ def check_union(expr, expr_info): "Base '%s' is not a valid base 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' must have a string " - "discriminator field" % 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, @@ -406,6 +427,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 check_type(expr_info, "Member '%s' of union '%s'" % (key, name), value, allow_array=True, @@ -439,6 +462,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: @@ -485,7 +510,8 @@ def check_struct(expr, expr_info): name = expr['type'] members = expr['data'] - check_type(expr_info, "'data' for type '%s'" % name, members) + check_type(expr_info, "'data' for type '%s'" % name, members, + allow_optional=True) check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'), allowed_metas=['struct'], allow_dict=False) @@ -676,8 +702,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" @@ -691,7 +720,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 66333a7..7418364 100644 --- a/tests/qapi-schema/bad-ident.json +++ b/tests/qapi-schema/bad-ident.json @@ -1,3 +1,3 @@ -# 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/flat-union-bad-discriminator.err b/tests/qapi-schema/flat-union-bad-discriminator.err index 4f0c798..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' must have a string discriminator field +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..df290ce 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:5: 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 4957c72..cbf0afa 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 f4b6bed..e69de29 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.out +++ b/tests/qapi-schema/flat-union-optional-discriminator.out @@ -1,5 +0,0 @@ -[OrderedDict([('enum', 'Enum'), ('data', ['one', 'two'])]), - OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))]), - OrderedDict([('union', 'MyUnion'), ('base', 'Base'), ('discriminator', '*switch'), ('data', OrderedDict([('one', 'int'), ('two', 'str')]))])] -[{'enum_name': 'Enum', 'enum_values': ['one', 'two']}] -[OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))])] 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}] -[]