[Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names

[Eric Blake <eblake@redhat.com>]

To make collision detection between member names easier, we
might as well reject all attempts to use anything that would
collide with our use of 'has_' as a flag for optional members.
Also, a later patch will introduce a named union 'u' for
holding the branch names of a qapi union in a separate
namespace from non-variant QMP members, at which point it
would collide with a 'u' member.  Fortunately, none of our
qapi files were using these names, so we can reserve them now.

Note that we cannot forbid 'u' everywhere (by adding the
rejection code to check_name()), because the existing QKeyCode
enum already uses it, and because qapi-schema-test.json
intentionally uses it as a branch name (we could feasibly have
a union with one branch per QKeyCode).  Also, forbidding 'has_'
in branch names will eventually disappear, once branch names
cannot collide with other names.  So instead, this code does
separate checks for check_type() (when iterating over a dict,
common to struct, command args, and event data) and for
unions.  We do NOT need to reserve the names for alternates,
because alternates do not have any QMP members alongside its
branch names.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v9: new patch
---
 docs/qapi-code-gen.txt                         |  5 ++++-
 scripts/qapi.py                                | 12 ++++++++++++
 tests/qapi-schema/args-name-has.err            |  1 +
 tests/qapi-schema/args-name-has.exit           |  2 +-
 tests/qapi-schema/args-name-has.json           |  7 +++----
 tests/qapi-schema/args-name-has.out            |  6 ------
 tests/qapi-schema/flat-union-clash-branch.err  |  1 +
 tests/qapi-schema/flat-union-clash-branch.exit |  2 +-
 tests/qapi-schema/flat-union-clash-branch.json |  7 ++++---
 tests/qapi-schema/flat-union-clash-branch.out  | 11 -----------
 tests/qapi-schema/qapi-schema-test.json        |  9 ++++-----
 tests/qapi-schema/struct-member-u.err          |  1 +
 tests/qapi-schema/struct-member-u.exit         |  2 +-
 tests/qapi-schema/struct-member-u.json         |  8 ++++----
 tests/qapi-schema/struct-member-u.out          |  3 ---
 15 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c4264a8..df2d198 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -112,7 +112,10 @@ and field names within a type, should be all lower case with words
 separated by a hyphen.  However, some existing older commands and
 complex types use underscore; when extending such expressions,
 consistency is preferred over blindly avoiding underscore.  Event
-names should be ALL_CAPS with words separated by underscore.
+names should be ALL_CAPS with words separated by underscore.  Field
+names cannot be 'u', as this is reserved for generated code for
+unions, nor can they start with 'has-' or 'has_', as this is
+reserved for tracking optional fields.

 Any name (command, event, type, field, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 09ba44b..1e7e08b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -488,6 +488,10 @@ def check_type(expr_info, source, value, allow_array=False,
     for (key, arg) in value.items():
         check_name(expr_info, "Member of %s" % source, key,
                    allow_optional=allow_optional)
+        if key == 'u' or key.startswith('has-') or key.startswith('has_'):
+            raise QAPIExprError(expr_info,
+                                "Member of %s uses reserved name '%s'"
+                                % (source, key))
         # Todo: allow dictionaries to represent default values of
         # an optional argument.
         check_type(expr_info, "Member '%s' of %s" % (key, source), arg,
@@ -588,6 +592,14 @@ def check_union(expr, expr_info):
     # Check every branch
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)
+        # TODO: As long as branch names can collide with QMP names, we
+        # must prevent branches starting with 'has_'. However, we do not
+        # need to reject 'u', because that is reserved for when we start
+        # sticking branch names in a C union named 'u'.
+        if key.startswith('has-') or key.startswith('has_'):
+            raise QAPIExprError(expr_info,
+                                "Branch of union '%s' uses reserved name '%s'"
+                                % (name, key))

         # Each value must name a known type; furthermore, in flat unions,
         # branches must be a struct with no overlapping member names
diff --git a/tests/qapi-schema/args-name-has.err b/tests/qapi-schema/args-name-has.err
index e69de29..cb57552 100644
--- a/tests/qapi-schema/args-name-has.err
+++ b/tests/qapi-schema/args-name-has.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-name-has.json:5: Member of 'data' for command 'oops' uses reserved name 'has-a'
diff --git a/tests/qapi-schema/args-name-has.exit b/tests/qapi-schema/args-name-has.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-name-has.exit
+++ b/tests/qapi-schema/args-name-has.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-name-has.json b/tests/qapi-schema/args-name-has.json
index a2197de..4297549 100644
--- a/tests/qapi-schema/args-name-has.json
+++ b/tests/qapi-schema/args-name-has.json
@@ -1,6 +1,5 @@
 # C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'has_a' members, one from the flag for optional 'a', and the other
-# from member 'has-a'.  Either reject this at parse time, or munge the C
-# names to avoid the collision.
+# This would attempt to create two 'has_a' members of the C struct, one
+# from the flag for optional 'a', and the other from member 'has-a'.
+# TODO we could munge the optional flag name to avoid the collision.
 { 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
diff --git a/tests/qapi-schema/args-name-has.out b/tests/qapi-schema/args-name-has.out
index 5a18b6b..e69de29 100644
--- a/tests/qapi-schema/args-name-has.out
+++ b/tests/qapi-schema/args-name-has.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops-arg
-    member a: str optional=True
-    member has-a: str optional=False
-command oops :obj-oops-arg -> None
-   gen=True success_response=True
diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
index e69de29..e6b6294 100644
--- a/tests/qapi-schema/flat-union-clash-branch.err
+++ b/tests/qapi-schema/flat-union-clash-branch.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-branch.json:13: Branch of union 'TestUnion' uses reserved name 'has-a'
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ b/tests/qapi-schema/flat-union-clash-branch.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
index c9f08e3..8efbcfd 100644
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -1,8 +1,9 @@
 # Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'has_a'
+# This is rejected because the C struct would have duplicate 'has_a'
 # (one as the implicit flag for the optional base member, the other from
-# the C member for the branch name).  We should either reject the collision
-# at parse time, or munge the generated branch name to allow this to compile.
+# the C member for the branch name).
+# TODO: we should munge generated branch names to not collide with the
+# non-variant struct members.
 { 'enum': 'TestEnum',
   'data': [ 'has-a' ] }
 { 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
index 1491081..e69de29 100644
--- a/tests/qapi-schema/flat-union-clash-branch.out
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -1,11 +0,0 @@
-object :empty
-object Base
-    member enum1: TestEnum optional=False
-    member a: str optional=True
-object Branch1
-    member string: str optional=False
-enum TestEnum ['has-a']
-object TestUnion
-    base Base
-    tag enum1
-    case has-a: Branch1
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 81d19d0..1ca7e4f 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -105,11 +105,10 @@
             'sizes': ['size'],
             'any': ['any'] } }

-# Even if 'u' is forbidden as a struct member name, it should still be
-# valid as a type or union branch name. And although '*Kind' and '*List'
-# are forbidden as type names, they should not be forbidden as a member
-# or branch name.
-# TODO - 'has_*' should also be forbidden as a member name.
+# Even though 'u' and 'has_*' are forbidden as struct member names, they
+# should still be valid as a type or union branch name. And although
+# '*Kind' and '*List' are forbidden as type names, they should not be
+# forbidden as a member or branch name.
 { 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
 { 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
                           'myList': 'has_a' } }
diff --git a/tests/qapi-schema/struct-member-u.err b/tests/qapi-schema/struct-member-u.err
index e69de29..5610310 100644
--- a/tests/qapi-schema/struct-member-u.err
+++ b/tests/qapi-schema/struct-member-u.err
@@ -0,0 +1 @@
+tests/qapi-schema/struct-member-u.json:6: Member of 'data' for struct 'Oops' uses reserved name 'u'
diff --git a/tests/qapi-schema/struct-member-u.exit b/tests/qapi-schema/struct-member-u.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/struct-member-u.exit
+++ b/tests/qapi-schema/struct-member-u.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/struct-member-u.json b/tests/qapi-schema/struct-member-u.json
index d72023d..a0e05b5 100644
--- a/tests/qapi-schema/struct-member-u.json
+++ b/tests/qapi-schema/struct-member-u.json
@@ -1,6 +1,6 @@
 # Potential C member name collision
-# FIXME - This parses and compiles, but would cause a collision if this
-# struct is later reworked to be part of a flat union, once unions hide
-# their tag values under a C union named 'u'. We should reject the use
-# of this identifier to reserve it for internal use.
+# We reject use of 'u' as a member name, to allow it for internal use in
+# putting union branch members in a separate namespace from QMP members.
+# This is true even for non-unions, because it is possible to convert a
+# struct to flat union while remaining backwards compatible in QMP.
 { 'struct': 'Oops', 'data': { 'u': 'str' } }
diff --git a/tests/qapi-schema/struct-member-u.out b/tests/qapi-schema/struct-member-u.out
index aa53e7f..e69de29 100644
--- a/tests/qapi-schema/struct-member-u.out
+++ b/tests/qapi-schema/struct-member-u.out
@@ -1,3 +0,0 @@
-object :empty
-object Oops
-    member u: str optional=False
-- 
2.4.3