[Qemu-devel] [PATCH v9 01/17] qapi: Add tests for reserved name abuse

[Eric Blake]

A future patch wants to change qapi union representation from
an anonymous C union over to a named C union 'u', so that the
C names of tag values are in a separate namespace and thus
cannot collide with the C names of non-variant QMP members.
But for that to not cause any problems with future extensions
to existing qapi, it would be best if we prohibit 'u' as a
member name everywhere, to reserve it for our internal use.
(Remember that although 'u' would only actually collide in
flat unions, we must also worry about the fact that it is
possible to convert from a struct to a flat union in a future
qemu version while remaining backwards-compatible in QMP.)

We are failing to detect a collision between a QMP member and
the implicit 'has_*' flag for another optional QMP member. The
easiest fix would be for a future patch to reserve the entire
"has[-_]" namespace for member names (also for branch names,
but only as long as branch names can collide with QMP names).

Our current representation of qapi arrays is done by appending
'List' to the element name; but we are not preventing the
creation of a non-array type with the same name.  It is also
possible to abuse the internal naming by writing such things
as ['intList'] for a 2-dimensional array of integers; however,
we may want to later add support for explicit 2D arrays such
as [['int']], so it is better to defer writing tests for what
we will permit and reject when it comes to multi-dimensioned
arrays until that later design is complete; for now, we are
only testing type collision.

On the other hand, there is no reason to forbid type name
patterns from member names, or member name patterns from types,
since the two are not in the same namespace in C and won't
collide.

Modify and add tests to cover these issues.

Signed-off-by: Eric Blake <address@hidden>

---
v9: new patch
---
 tests/Makefile                                 |  3 +++
 tests/qapi-schema/args-name-has.err            |  0
 tests/qapi-schema/args-name-has.exit           |  1 +
 tests/qapi-schema/args-name-has.json           |  6 ++++++
 tests/qapi-schema/args-name-has.out            |  6 ++++++
 tests/qapi-schema/flat-union-clash-branch.json | 17 +++++++----------
 tests/qapi-schema/flat-union-clash-branch.out  |  9 +++------
 tests/qapi-schema/qapi-schema-test.json        |  9 +++++++++
 tests/qapi-schema/qapi-schema-test.out         | 12 ++++++++++++
 tests/qapi-schema/struct-member-u.err          |  0
 tests/qapi-schema/struct-member-u.exit         |  1 +
 tests/qapi-schema/struct-member-u.json         |  6 ++++++
 tests/qapi-schema/struct-member-u.out          |  3 +++
 tests/qapi-schema/struct-name-list.err         |  0
 tests/qapi-schema/struct-name-list.exit        |  1 +
 tests/qapi-schema/struct-name-list.json        |  5 +++++
 tests/qapi-schema/struct-name-list.out         |  3 +++
 17 files changed, 66 insertions(+), 16 deletions(-)
 create mode 100644 tests/qapi-schema/args-name-has.err
 create mode 100644 tests/qapi-schema/args-name-has.exit
 create mode 100644 tests/qapi-schema/args-name-has.json
 create mode 100644 tests/qapi-schema/args-name-has.out
 create mode 100644 tests/qapi-schema/struct-member-u.err
 create mode 100644 tests/qapi-schema/struct-member-u.exit
 create mode 100644 tests/qapi-schema/struct-member-u.json
 create mode 100644 tests/qapi-schema/struct-member-u.out
 create mode 100644 tests/qapi-schema/struct-name-list.err
 create mode 100644 tests/qapi-schema/struct-name-list.exit
 create mode 100644 tests/qapi-schema/struct-name-list.json
 create mode 100644 tests/qapi-schema/struct-name-list.out

diff --git a/tests/Makefile b/tests/Makefile
index cb221de..ef2a19f 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -241,6 +241,7 @@ qapi-schema += args-invalid.json
 qapi-schema += args-member-array-bad.json
 qapi-schema += args-member-unknown.json
 qapi-schema += args-name-clash.json
+qapi-schema += args-name-has.json
 qapi-schema += args-union.json
 qapi-schema += args-unknown.json
 qapi-schema += bad-base.json
@@ -323,6 +324,8 @@ qapi-schema += struct-base-clash-deep.json
 qapi-schema += struct-base-clash.json
 qapi-schema += struct-data-invalid.json
 qapi-schema += struct-member-invalid.json
+qapi-schema += struct-member-u.json
+qapi-schema += struct-name-list.json
 qapi-schema += trailing-comma-list.json
 qapi-schema += trailing-comma-object.json
 qapi-schema += type-bypass-bad-gen.json
diff --git a/tests/qapi-schema/args-name-has.err 
b/tests/qapi-schema/args-name-has.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-name-has.exit 
b/tests/qapi-schema/args-name-has.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/args-name-has.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/args-name-has.json 
b/tests/qapi-schema/args-name-has.json
new file mode 100644
index 0000000..a2197de
--- /dev/null
+++ b/tests/qapi-schema/args-name-has.json
@@ -0,0 +1,6 @@
+# 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.
+{ '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
new file mode 100644
index 0000000..5a18b6b
--- /dev/null
+++ b/tests/qapi-schema/args-name-has.out
@@ -0,0 +1,6 @@
+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.json 
b/tests/qapi-schema/flat-union-clash-branch.json
index e593336..c9f08e3 100644
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -1,18 +1,15 @@
 # Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
-# (one from the base member, the other from the branch name).  We should
-# either reject the collision at parse time, or munge the generated branch
-# name to allow this to compile.
+# FIXME: this parses, but then fails to compile due to a 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.
 { 'enum': 'TestEnum',
-  'data': [ 'base', 'c-d' ] }
+  'data': [ 'has-a' ] }
 { 'struct': 'Base',
-  'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
+  'data': { 'enum1': 'TestEnum', '*a': 'str' } }
 { 'struct': 'Branch1',
   'data': { 'string': 'str' } }
-{ 'struct': 'Branch2',
-  'data': { 'value': 'int' } }
 { 'union': 'TestUnion',
   'base': 'Base',
   'discriminator': 'enum1',
-  'data': { 'base': 'Branch1',
-            'c-d': 'Branch2' } }
+  'data': { 'has-a': 'Branch1' } }
diff --git a/tests/qapi-schema/flat-union-clash-branch.out 
b/tests/qapi-schema/flat-union-clash-branch.out
index 8e0da73..1491081 100644
--- a/tests/qapi-schema/flat-union-clash-branch.out
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -1,14 +1,11 @@
 object :empty
 object Base
     member enum1: TestEnum optional=False
-    member c_d: str optional=True
+    member a: str optional=True
 object Branch1
     member string: str optional=False
-object Branch2
-    member value: int optional=False
-enum TestEnum ['base', 'c-d']
+enum TestEnum ['has-a']
 object TestUnion
     base Base
     tag enum1
-    case base: Branch1
-    case c-d: Branch2
+    case has-a: Branch1
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 4e2d7c2..c842e22 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -105,6 +105,15 @@
             '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' is forbidden
+# as a type name, it should not be forbidden as a member or branch name.
+# TODO - '*List' should also be forbidden as a type name, and 'has_*' as
+# a member name.
+{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
+{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
+                          'myList': 'has_a' } }
+
 # testing commands
 { 'command': 'user_def_cmd', 'data': {} }
 { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index a6c80e0..30c3ff0 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -22,6 +22,8 @@ object :obj-guest-get-time-arg
     member b: int optional=True
 object :obj-guest-sync-arg
     member arg: any optional=False
+object :obj-has_a-wrapper
+    member data: has_a optional=False
 object :obj-int16List-wrapper
     member data: int16List optional=False
 object :obj-int32List-wrapper
@@ -46,6 +48,8 @@ object :obj-uint32List-wrapper
     member data: uint32List optional=False
 object :obj-uint64List-wrapper
     member data: uint64List optional=False
+object :obj-uint8-wrapper
+    member data: uint8 optional=False
 object :obj-uint8List-wrapper
     member data: uint8List optional=False
 object :obj-user_def_cmd1-arg
@@ -191,6 +195,14 @@ command guest-get-time :obj-guest-get-time-arg -> int
    gen=True success_response=True
 command guest-sync :obj-guest-sync-arg -> any
    gen=True success_response=True
+object has_a
+    member MyKind: int optional=False
+    member MyList: intList optional=False
+object u
+    case u: :obj-uint8-wrapper
+    case myKind: :obj-has_a-wrapper
+    case myList: :obj-has_a-wrapper
+enum uKind ['u', 'myKind', 'myList']
 command user_def_cmd None -> None
    gen=True success_response=True
 command user_def_cmd1 :obj-user_def_cmd1-arg -> None
diff --git a/tests/qapi-schema/struct-member-u.err 
b/tests/qapi-schema/struct-member-u.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/struct-member-u.exit 
b/tests/qapi-schema/struct-member-u.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/struct-member-u.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/struct-member-u.json 
b/tests/qapi-schema/struct-member-u.json
new file mode 100644
index 0000000..d72023d
--- /dev/null
+++ b/tests/qapi-schema/struct-member-u.json
@@ -0,0 +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.
+{ 'struct': 'Oops', 'data': { 'u': 'str' } }
diff --git a/tests/qapi-schema/struct-member-u.out 
b/tests/qapi-schema/struct-member-u.out
new file mode 100644
index 0000000..aa53e7f
--- /dev/null
+++ b/tests/qapi-schema/struct-member-u.out
@@ -0,0 +1,3 @@
+object :empty
+object Oops
+    member u: str optional=False
diff --git a/tests/qapi-schema/struct-name-list.err 
b/tests/qapi-schema/struct-name-list.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/struct-name-list.exit 
b/tests/qapi-schema/struct-name-list.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/struct-name-list.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/struct-name-list.json 
b/tests/qapi-schema/struct-name-list.json
new file mode 100644
index 0000000..8ad38e6
--- /dev/null
+++ b/tests/qapi-schema/struct-name-list.json
@@ -0,0 +1,5 @@
+# Potential C name collision
+# FIXME - This parses and compiles on its own, but prevents the user from
+# creating a type named 'Foo' and using ['Foo'] for an array.  We should
+# reject the use of any non-array type names ending in 'List'.
+{ 'struct': 'FooList', 'data': { 's': 'str' } }
diff --git a/tests/qapi-schema/struct-name-list.out 
b/tests/qapi-schema/struct-name-list.out
new file mode 100644
index 0000000..0406bfe
--- /dev/null
+++ b/tests/qapi-schema/struct-name-list.out
@@ -0,0 +1,3 @@
+object :empty
+object FooList
+    member s: str optional=False
-- 
2.4.3