From patchwork Wed Nov 18 22:55:44 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 546241 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 07134141420 for ; Thu, 19 Nov 2015 09:56:15 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; secure) header.d=comcast.net header.i=@comcast.net header.b=EZsU9U2N; dkim-atps=neutral Received: from localhost ([::1]:38703 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzBe9-0006BQ-3a for incoming@patchwork.ozlabs.org; Wed, 18 Nov 2015 17:56:13 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60983) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzBdr-0005tU-O8 for qemu-devel@nongnu.org; Wed, 18 Nov 2015 17:55:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzBdo-0004Md-Fg for qemu-devel@nongnu.org; Wed, 18 Nov 2015 17:55:55 -0500 Received: from resqmta-po-10v.sys.comcast.net ([96.114.154.169]:53953) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzBdo-0004Lu-6y for qemu-devel@nongnu.org; Wed, 18 Nov 2015 17:55:52 -0500 Received: from resomta-po-08v.sys.comcast.net ([96.114.154.232]) by resqmta-po-10v.sys.comcast.net with comcast id jAvC1r002516pyw01AvqAU; Wed, 18 Nov 2015 22:55:50 +0000 Received: from red.redhat.com ([24.10.254.122]) by resomta-po-08v.sys.comcast.net with comcast id jAvn1r0052fD5rL01AvqDN; Wed, 18 Nov 2015 22:55:50 +0000 From: Eric Blake To: qemu-devel@nongnu.org Date: Wed, 18 Nov 2015 15:55:44 -0700 Message-Id: <1447887345-1170-1-git-send-email-eblake@redhat.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <1447836791-369-28-git-send-email-eblake@redhat.com> References: <1447836791-369-28-git-send-email-eblake@redhat.com> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=comcast.net; s=q20140121; t=1447887350; bh=NxAafG3n5Dbe8kFQwqFeYdDDzQ6RdvriHYKS6ujGsZs=; h=Received:Received:From:To:Subject:Date:Message-Id; b=EZsU9U2NtN2QPHuGUWPCmSP0UGlmylNrrqY7udyC0fcrruZAVfim+r9nKdcspe3bU qIdRvpKM3llLFRazV+ag4vE6zilYFsp9MAILJn4KFGDaoc1MrxMStFXD6r1MbtnM1w p5Ukp7uWnmExNSF3RgkulZtdjFj7bYKfLOm9zoYfV/rG9Lw+/nE8Y3GKXf29yUF25k OKEOcUqreudTTNIo68pfOtQ2iSUoCVvEgqMI/QFo3oWH/EoEOvpzZOb+CxlvQisaw6 3MoY0CgSTPCRKo+7smJSLJdfH95W8m3rzurZ2JRHNlgNk7kLnCIfdB6ArTo/Ded0xF Zn6FhdvxOTlZQ== X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 96.114.154.169 Cc: armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [PATCH] fixup! qapi: Forbid case-insensitive clashes 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 [Replace the old commit message with this:] qapi: Forbid case-insensitive clashes In general, designing user interfaces that rely on case distinction is poor practice. Another benefit of enforcing a restriction against case-insensitive clashes is that we no longer have to worry about the situation of enum values that could be distinguished by case if mapped by c_name(), but which cannot be distinguished when mapped to C as ALL_CAPS by c_enum_const() [via c_name(name, False).upper()]. Thus, having the generator look for case collisions up front will prevent developers from worrying whether different munging rules for member names compared to enum values as a discriminator will cause any problems in qapi unions. We do not have to care about c_name(name, True) vs. c_name(name, False), as long as any pair of munged names being compared were munged using the same flag value to c_name(). This is because commit 9fb081e already reserved names munging to 'q_', and this patch extends the reservation to 'Q_'; and because recent patches fixed things to ensure the only thing done by the flag is adding the prefix 'q_', that the only use of '.' (which also munges to '_') is in downstream extension prefixes, and that enum munging does not add underscores to any CamelCase values. However, we DO have to care about the fact that we have a command 'stop' and an event 'STOP' (currently the only case collision of any of our .json entities). To solve that, add a new QAPISchemaEntity.c_namecase() that returns a munged version of the name that can be used in checking for case clashes, and override it in QAPISchemaEvent to use a different case (thus, indexing by .c_namecase() will create two natural key partitions), then add a new _clash_dict to QAPISchema._def_entity() that checks for case collisions after ruling out identical spellings. This works because all entities have at least a leading letter in their name. We could go further and enforce that events are named with 'ALL_CAPS' and that nothing else is named in that manner; but that can be done as a followup if desired. We could also separate commands from type names (the way we already separated events into a separate namespace), but then we'd have to make .c_namecase() pick something other than the convenient upper() vs. lower() in order to partition dictionary keys into three sets (perhaps a leading digit, since all entities start with a letter). There is also the possibility that we may want to add a future extension to QMP of teaching it to be case-insensitive (the user could request command 'Quit' instead of 'quit', or could spell a struct field as 'CPU' instead of 'cpu'). But for that to be a practical extension, we cannot break backwards compatibility with any existing struct that was already relying on case sensitivity. Fortunately, after a recent patch cleaned up CpuInfo, there are no such existing qapi structs. Of course, the idea of a future extension is not as strong of a reason to make the change. At any rate, it is easier to be strict now, and relax things later if we find a reason to need case-sensitive QMP members, than it would be to wish we had the restriction in place. Add new tests args-case-clash.json and command-type-case-clash.json to demonstrate that the collision detection works. Signed-off-by: Eric Blake --- Use a separate clash dictionary, fix a typo that we couldn't trigger, add .c_namecase(), no longer need to use attrgetter --- scripts/qapi.py | 48 ++++++++++++++++++------------------------------ 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 5207c12..114e07a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -13,7 +13,6 @@ import re from ordereddict import OrderedDict -from operator import attrgetter import errno import getopt import os @@ -381,7 +380,7 @@ def check_name(expr_info, source, name, allow_optional=False, membername = 'D' + membername # Reserve the entire 'q_'/'Q_' namespace for c_name() if not valid_name.match(membername) or \ - c_name(membername, False).upper().startswith('Q_'): + c_name(membername, False).lower().startswith('q_'): raise QAPIExprError(expr_info, "%s uses invalid name '%s'" % (source, name)) @@ -801,6 +800,9 @@ class QAPISchemaEntity(object): def c_name(self): return c_name(self.name) + def c_namecase(self): + return c_name(self.name).lower() + def check(self, schema): pass @@ -1041,7 +1043,7 @@ class QAPISchemaObjectTypeMember(object): assert self.type def check_clash(self, info, seen): - cname = c_name(self.name).upper() + cname = c_name(self.name).lower() if cname in seen: raise QAPIExprError(info, "%s collides with %s" @@ -1086,7 +1088,7 @@ class QAPISchemaObjectTypeVariants(object): def check(self, schema, seen): if not self.tag_member: # flat union - self.tag_member = seen[c_name(self.tag_name).upper()] + self.tag_member = seen[c_name(self.tag_name).lower()] assert self.tag_name == self.tag_member.name assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: @@ -1185,14 +1187,16 @@ class QAPISchemaEvent(QAPISchemaEntity): def visit(self, visitor): visitor.visit_event(self.name, self.info, self.arg_type) + def c_namecase(self): + return c_name(self.name).upper() + class QAPISchema(object): def __init__(self, fname): try: self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs) - # _entity_dict holds two namespaces: events are stored via - # name.upper(), commands and types are stored via name.lower(). self._entity_dict = {} + self._clash_dict = {} self._predefining = True self._def_predefineds() self._predefining = False @@ -1205,32 +1209,17 @@ class QAPISchema(object): def _def_entity(self, ent): # Only the predefined types are allowed to not have info assert ent.info or self._predefining - # On insertion, we need to check for an exact match in either - # namespace, then for case collision in a single namespace - if self.lookup_entity(ent.name): - raise QAPIExprError(ent.info, - "Entity '%s' already defined" % end.name) - cname = c_name(ent.name) - if isinstance(ent, QAPISchemaEvent): - cname = cname.upper() - else: - cname = cname.lower() - if cname in self._entity_dict: + assert ent.name not in self._entity_dict + cname = ent.c_namecase() + if cname in self._clash_dict: raise QAPIExprError(ent.info, "Entity '%s' collides with '%s'" - % (ent.name, self._entity_dict[cname].name)) - self._entity_dict[cname] = ent + % (ent.name, self._clash_dict[cname])) + self._entity_dict[ent.name] = ent + self._clash_dict[cname] = ent.name def lookup_entity(self, name, typ=None): - # No thanks to 'stop'/'STOP', we have to check two namespaces during - # lookups, but only return a result on exact match. - ent1 = self._entity_dict.get(c_name(name).lower()) # commands, types - ent2 = self._entity_dict.get(c_name(name).upper()) # events - ent = None - if ent1 and ent1.name == name: - ent = ent1 - elif ent2 and ent2.name == name: - ent = ent2 + ent = self._entity_dict.get(name) if typ and not isinstance(ent, typ): return None return ent @@ -1414,8 +1403,7 @@ class QAPISchema(object): def visit(self, visitor): visitor.visit_begin(self) - for entity in sorted(self._entity_dict.values(), - key=attrgetter('name')): + for (name, entity) in sorted(self._entity_dict.items()): if visitor.visit_needed(entity): entity.visit(visitor) visitor.visit_end()