[Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes

[Eric Blake]

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, use
some tricks in the lookup map for entities.  With some careful
choice of keys, we can bisect the map into two collision pools
(name.upper() for events, name.lower() for all else).  Since
we already document that no two entities can have the exact
same spelling, an entity can only occur under one of the two
partitions of the map.  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,
but then we no longer have a convenient upper vs. lower
partitioning allowing us to share a single dictionary.

In order to keep things stable, the visit() method has to
ensure that it visits entities sorted by their real name, and
not by the new munged keys of the dictionary; Python's
attrgetter is a lifesaver for this task.

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 <address@hidden>

---
v12: add in entity case collisions (required sharing two maps),
improve commit message
v11: rebase to latest, don't focus so hard on future case-insensitive
extensions, adjust commit message
v10: new patch
---
 docs/qapi-code-gen.txt                         |  7 +++++
 scripts/qapi.py                                | 41 +++++++++++++++++++++-----
 tests/Makefile                                 |  2 ++
 tests/qapi-schema/args-case-clash.err          |  1 +
 tests/qapi-schema/args-case-clash.exit         |  1 +
 tests/qapi-schema/args-case-clash.json         |  4 +++
 tests/qapi-schema/args-case-clash.out          |  0
 tests/qapi-schema/command-type-case-clash.err  |  1 +
 tests/qapi-schema/command-type-case-clash.exit |  1 +
 tests/qapi-schema/command-type-case-clash.json |  3 ++
 tests/qapi-schema/command-type-case-clash.out  |  0
 11 files changed, 53 insertions(+), 8 deletions(-)
 create mode 100644 tests/qapi-schema/args-case-clash.err
 create mode 100644 tests/qapi-schema/args-case-clash.exit
 create mode 100644 tests/qapi-schema/args-case-clash.json
 create mode 100644 tests/qapi-schema/args-case-clash.out
 create mode 100644 tests/qapi-schema/command-type-case-clash.err
 create mode 100644 tests/qapi-schema/command-type-case-clash.exit
 create mode 100644 tests/qapi-schema/command-type-case-clash.json
 create mode 100644 tests/qapi-schema/command-type-case-clash.out

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index b01a691..1f6cb16 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -102,6 +102,13 @@ single-dimension array of that type; multi-dimension 
arrays are not
 directly supported (although an array of a complex struct that
 contains an array member is possible).

+Client JSON Protocol is case-sensitive.  However, the generator
+rejects attempts to create object members that differ only in case
+after normalization (thus 'a-b' and 'A_B' collide); and likewise
+rejects attempts to create commands or types that differ only in case,
+or events that differ only in case (it is possible to have a command
+and event map to the same case-insensitive name, though).
+
 Types, commands, and events share a common namespace.  Therefore,
 generally speaking, type definitions should always use CamelCase for
 user-defined type names, while built-in types are lowercase. Type
diff --git a/scripts/qapi.py b/scripts/qapi.py
index cde15f2..e41dbaf 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -13,6 +13,7 @@

 import re
 from ordereddict import OrderedDict
+from operator import attrgetter
 import errno
 import getopt
 import os
@@ -378,9 +379,9 @@ def check_name(expr_info, source, name, 
allow_optional=False,
     # code always prefixes it with the enum name
     if enum_member and membername[0].isdigit():
         membername = 'D' + membername
-    # Reserve the entire 'q_' namespace for c_name()
+    # Reserve the entire 'q_'/'Q_' namespace for c_name()
     if not valid_name.match(membername) or \
-       c_name(membername, False).startswith('q_'):
+       c_name(membername, False).upper().startswith('Q_'):
         raise QAPIExprError(expr_info,
                             "%s uses invalid name '%s'" % (source, name))

@@ -1040,7 +1041,7 @@ class QAPISchemaObjectTypeMember(object):
         assert self.type

     def check_clash(self, info, seen):
-        cname = c_name(self.name)
+        cname = c_name(self.name).upper()
         if cname in seen:
             raise QAPIExprError(info,
                                 "%s collides with %s"
@@ -1085,7 +1086,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)]
+            self.tag_member = seen[c_name(self.tag_name).upper()]
             assert self.tag_name == self.tag_member.name
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
@@ -1189,6 +1190,8 @@ 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._predefining = True
             self._def_predefineds()
@@ -1202,11 +1205,32 @@ class QAPISchema(object):
     def _def_entity(self, ent):
         # Only the predefined types are allowed to not have info
         assert ent.info or self._predefining
-        assert ent.name not in self._entity_dict
-        self._entity_dict[ent.name] = ent
+        # 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:
+            raise QAPIExprError(ent.info,
+                                "Entity '%s' collides with '%s'"
+                                % (ent.name, self._entity_dict[cname].name))
+        self._entity_dict[cname] = ent

     def lookup_entity(self, name, typ=None):
-        ent = self._entity_dict.get(name)
+        # 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
         if typ and not isinstance(ent, typ):
             return None
         return ent
@@ -1390,7 +1414,8 @@ class QAPISchema(object):

     def visit(self, visitor):
         visitor.visit_begin(self)
-        for (name, entity) in sorted(self._entity_dict.items()):
+        for entity in sorted(self._entity_dict.values(),
+                             key=attrgetter('name')):
             if visitor.visit_needed(entity):
                 entity.visit(visitor)
         visitor.visit_end()
diff --git a/tests/Makefile b/tests/Makefile
index a8a3b12..762b0ca 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -242,6 +242,7 @@ qapi-schema += args-alternate.json
 qapi-schema += args-any.json
 qapi-schema += args-array-empty.json
 qapi-schema += args-array-unknown.json
+qapi-schema += args-case-clash.json
 qapi-schema += args-int.json
 qapi-schema += args-invalid.json
 qapi-schema += args-member-array-bad.json
@@ -256,6 +257,7 @@ qapi-schema += bad-type-bool.json
 qapi-schema += bad-type-dict.json
 qapi-schema += bad-type-int.json
 qapi-schema += command-int.json
+qapi-schema += command-type-case-clash.json
 qapi-schema += comments.json
 qapi-schema += double-data.json
 qapi-schema += double-type.json
diff --git a/tests/qapi-schema/args-case-clash.err 
b/tests/qapi-schema/args-case-clash.err
new file mode 100644
index 0000000..0fafe75
--- /dev/null
+++ b/tests/qapi-schema/args-case-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-case-clash.json:4: 'A' (parameter of oops) collides 
with 'a' (parameter of oops)
diff --git a/tests/qapi-schema/args-case-clash.exit 
b/tests/qapi-schema/args-case-clash.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-case-clash.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-case-clash.json 
b/tests/qapi-schema/args-case-clash.json
new file mode 100644
index 0000000..e6f0625
--- /dev/null
+++ b/tests/qapi-schema/args-case-clash.json
@@ -0,0 +1,4 @@
+# C member name collision
+# Reject members that clash case-insensitively, even though our mapping to
+# C names preserves case.
+{ 'command': 'oops', 'data': { 'a': 'str', 'A': 'int' } }
diff --git a/tests/qapi-schema/args-case-clash.out 
b/tests/qapi-schema/args-case-clash.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/command-type-case-clash.err 
b/tests/qapi-schema/command-type-case-clash.err
new file mode 100644
index 0000000..b1cc697
--- /dev/null
+++ b/tests/qapi-schema/command-type-case-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/command-type-case-clash.json:3: Entity 'foo' collides with 
'Foo'
diff --git a/tests/qapi-schema/command-type-case-clash.exit 
b/tests/qapi-schema/command-type-case-clash.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/command-type-case-clash.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/command-type-case-clash.json 
b/tests/qapi-schema/command-type-case-clash.json
new file mode 100644
index 0000000..1f6adee
--- /dev/null
+++ b/tests/qapi-schema/command-type-case-clash.json
@@ -0,0 +1,3 @@
+# we reject commands that would differ only case from a type
+{ 'struct': 'Foo', 'data': { 'i': 'int' } }
+{ 'command': 'foo', 'data': { 'character': 'str' } }
diff --git a/tests/qapi-schema/command-type-case-clash.out 
b/tests/qapi-schema/command-type-case-clash.out
new file mode 100644
index 0000000..e69de29
-- 
2.4.3