From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56975) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn3yE-0005en-R7 for qemu-devel@nongnu.org; Tue, 28 Apr 2015 07:46:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yn3yB-0001x3-Cv for qemu-devel@nongnu.org; Tue, 28 Apr 2015 07:46:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45174) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn3yB-0001wl-5Q for qemu-devel@nongnu.org; Tue, 28 Apr 2015 07:46:31 -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 93A348E765 for ; Tue, 28 Apr 2015 11:46:30 +0000 (UTC) From: Markus Armbruster References: <1428206887-7921-1-git-send-email-eblake@redhat.com> <1428206887-7921-26-git-send-email-eblake@redhat.com> Date: Tue, 28 Apr 2015 13:46:27 +0200 In-Reply-To: <1428206887-7921-26-git-send-email-eblake@redhat.com> (Eric Blake's message of "Sat, 4 Apr 2015 22:07:56 -0600") Message-ID: <87vbggqxv0.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v6 25/36] qapi: Require valid names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.co, qemu-devel@nongnu.org Eric Blake writes: > 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 > > The set of valid names includes [a-zA-Z_][a-zA-Z0-9._-]* (where > '.' is useful only in downstream extensions). Since we have > existing enum values beginning with a digit (see QKeyCode), a Meh. Didn't see the patch, or else I would've shot down these names. Too late now. > small hack is used to pass the same regex. A bit vague. > > Signed-off-by: Eric Blake > > --- > > v6: rebase onto earlier changes; use regex instead of sets, and > ensure leading letter or _; don't force event case; drop dead > code for exempting '**'; extend coverage to enum members > --- > 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 9f64a0d..9b97683 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 This is the hack. Less vague commit message: Valid names match [a-zA-Z_][a-zA-Z0-9._-]*. Except for enumeration names, which match [a-zA-Z0-9._-]+. By convention, '.' is used only in downstream extensions. > + 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 > We could reject new enumeration names beginning with a digit with a whitelist, similar to how we reject new commands returning non-dictionaries in the next patch. Probably not worth the bother. Reviewed-by: Markus Armbruster