[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 <ebl...@redhat.com>
---
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()
--
2.4.3
