From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47719) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbXqB-00036X-LJ for qemu-devel@nongnu.org; Fri, 27 Mar 2015 13:14:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbXq5-00071c-Ed for qemu-devel@nongnu.org; Fri, 27 Mar 2015 13:14:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41214) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbXq5-00071X-6m for qemu-devel@nongnu.org; Fri, 27 Mar 2015 13:14:33 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-22-git-send-email-eblake@redhat.com> Date: Fri, 27 Mar 2015 18:14:29 +0100 In-Reply-To: <1427227433-5030-22-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:46 -0600") Message-ID: <87oane8iru.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 21/28] qapi: Require valid names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com Eric Blake writes: > 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 I'm afraid this conditional is superfluous or wrong. Our schemata don't trigger it. As far as I know, '**' may occur as pseudo-type in a command's 'data' or 'returns', and nowhere else. Example 1 (qom-get): 'returns': '**' Example 2 (qom-set): 'data': { 'path': 'str', 'property': 'str', 'value': '**' }, Example 3 (hypothetical) 'returns': { 'frob': '**' } Both 'data' and 'returns' are checked by check_type(). Example 1 takes the early exit on data = '**' there. Example 2 goes through this loop 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_optional=True) The '**' is fed to check_type(), *not* to check_name(). check_type() takes the same early exit. [...]