Eric Blake <ebl...@redhat.com> writes:
> 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 (the collision is also
> possible for branch names within flat unions, but only as long
> as branch names can collide with QMP names; since future
> patches are about to remove that, it is not worth testing here).
This is args-has-clash.json.
> A similar collision exists between a QMP member where c_name()
> munges what might otherwise be a reserved name to start with
> 'q_', and another member explicitly starts with "q[-_]". Again,
> the easiest task for a future patch will be reserving the entire
s/task/solution/ ?
> namespace, but here for commands as well as members.
This is reserved-member.json.
> 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.
This is struct-name-list.json.
> Finally, our testsuite does not have any compilation coverage
> of struct inheritance with empty qapi structs.
This is qapi-schema-test.json.
Left undescribed: reserved-commands.json :)
> Add tests to cover these issues.
>
> On the other hand, there is currently no technical 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 (but it's not worth adding positive tests
> of these corner cases, especially while there is other churn
> pending in patches that rearrange which collisions actually
> happen).
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v10: retitle, split off 'u' collisions and positive tests for
> later, add 'q_' collisions and empty struct inheritance, improve
> commit message, rename args-name-has to args-has-clash
> v9: new patch
> ---
> tests/Makefile | 4 ++++
> tests/qapi-schema/args-has-clash.err | 0
> tests/qapi-schema/args-has-clash.exit | 1 +
> tests/qapi-schema/args-has-clash.json | 6 ++++++
> tests/qapi-schema/args-has-clash.out | 6 ++++++
> tests/qapi-schema/qapi-schema-test.json | 4 ++++
> tests/qapi-schema/qapi-schema-test.out | 3 +++
> tests/qapi-schema/reserved-command.err | 0
> tests/qapi-schema/reserved-command.exit | 1 +
> tests/qapi-schema/reserved-command.json | 7 +++++++
> tests/qapi-schema/reserved-command.out | 5 +++++
> tests/qapi-schema/reserved-member.err | 0
> tests/qapi-schema/reserved-member.exit | 1 +
> tests/qapi-schema/reserved-member.json | 6 ++++++
> tests/qapi-schema/reserved-member.out | 4 ++++
> 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 +++
> 19 files changed, 57 insertions(+)
> create mode 100644 tests/qapi-schema/args-has-clash.err
> create mode 100644 tests/qapi-schema/args-has-clash.exit
> create mode 100644 tests/qapi-schema/args-has-clash.json
> create mode 100644 tests/qapi-schema/args-has-clash.out
> create mode 100644 tests/qapi-schema/reserved-command.err
> create mode 100644 tests/qapi-schema/reserved-command.exit
> create mode 100644 tests/qapi-schema/reserved-command.json
> create mode 100644 tests/qapi-schema/reserved-command.out
> create mode 100644 tests/qapi-schema/reserved-member.err
> create mode 100644 tests/qapi-schema/reserved-member.exit
> create mode 100644 tests/qapi-schema/reserved-member.json
> create mode 100644 tests/qapi-schema/reserved-member.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 0531b30..cc6b24f 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -237,6 +237,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-has-clash.json
> qapi-schema += args-int.json
> qapi-schema += args-invalid.json
> qapi-schema += args-member-array-bad.json
> @@ -314,6 +315,8 @@ qapi-schema += redefined-builtin.json
> qapi-schema += redefined-command.json
> qapi-schema += redefined-event.json
> qapi-schema += redefined-type.json
> +qapi-schema += reserved-command.json
> +qapi-schema += reserved-member.json
> qapi-schema += returns-alternate.json
> qapi-schema += returns-array-bad.json
> qapi-schema += returns-dict.json
> @@ -324,6 +327,7 @@ 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-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-has-clash.err
> b/tests/qapi-schema/args-has-clash.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/args-has-clash.exit
> b/tests/qapi-schema/args-has-clash.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/args-has-clash.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/args-has-clash.json
> b/tests/qapi-schema/args-has-clash.json
> new file mode 100644
> index 0000000..a2197de
> --- /dev/null
> +++ b/tests/qapi-schema/args-has-clash.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-has-clash.out
> b/tests/qapi-schema/args-has-clash.out
> new file mode 100644
> index 0000000..5a18b6b
> --- /dev/null
> +++ b/tests/qapi-schema/args-has-clash.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/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index 4e2d7c2..48e104b 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -11,6 +11,10 @@
> # An empty enum, although unusual, is currently acceptable
> { 'enum': 'MyEnum', 'data': [ ] }
>
> +# Likewise for an empty struct, including an empty base
> +{ 'struct': 'Empty1', 'data': { } }
> +{ 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }
> +
> # for testing override of default naming heuristic
> { 'enum': 'QEnumTwo',
> 'prefix': 'QENUM_TWO',
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index a6c80e0..a7e9aab 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -81,6 +81,9 @@ event EVENT_A None
> event EVENT_B None
> event EVENT_C :obj-EVENT_C-arg
> event EVENT_D :obj-EVENT_D-arg
> +object Empty1
> +object Empty2
> + base Empty1
> enum EnumOne ['value1', 'value2', 'value3']
> object EventStructOne
> member struct1: UserDefOne optional=False
> diff --git a/tests/qapi-schema/reserved-command.err
> b/tests/qapi-schema/reserved-command.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/reserved-command.exit
> b/tests/qapi-schema/reserved-command.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-command.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/reserved-command.json
> b/tests/qapi-schema/reserved-command.json
> new file mode 100644
> index 0000000..be9944c
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-command.json
> @@ -0,0 +1,7 @@
> +# C entity name collision
> +# FIXME - This parses, but fails to compile, because it attempts to declare
> +# two 'qmp_q_unix' functions (one for 'q-unix', the other because c_name()
> +# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
> +# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
> +{ 'command': 'unix' }
> +{ 'command': 'q-unix' }
Note that mangling 'unix' to 'q-unix' is pretty pointless for command
names, because their C name occurs only in identifiers qmp_CNAME() and
qmp_marshal_CNAME().
> diff --git a/tests/qapi-schema/reserved-command.out
> b/tests/qapi-schema/reserved-command.out
> new file mode 100644
> index 0000000..b31b38f
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-command.out
> @@ -0,0 +1,5 @@
> +object :empty
> +command q-unix None -> None
> + gen=True success_response=True
> +command unix None -> None
> + gen=True success_response=True
> diff --git a/tests/qapi-schema/reserved-member.err
> b/tests/qapi-schema/reserved-member.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/reserved-member.exit
> b/tests/qapi-schema/reserved-member.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/reserved-member.json
> b/tests/qapi-schema/reserved-member.json
> new file mode 100644
> index 0000000..1602ed3
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member.json
> @@ -0,0 +1,6 @@
> +# C member name collision
> +# FIXME - This parses, but fails to compile, because it attempts to declare
> +# two 'q_unix' members (one for 'q-unix', the other because c_name()
> +# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
> +# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
> +{ 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } }
Unlike command names, member names actually occur by themselves, so some
name mangling is required.
A less ham-fisted approach would mangle *complete* identifiers, i.e.
c_name('qmp_' + name) instead of 'qmp_' + c_name(name).
Please feel free to stick to ham-fisted for now. We need to make
progress flushing the queue.
> diff --git a/tests/qapi-schema/reserved-member.out
> b/tests/qapi-schema/reserved-member.out
> new file mode 100644
> index 0000000..0d8685a
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member.out
> @@ -0,0 +1,4 @@
> +object :empty
> +object Foo
> + member unix: int optional=False
> + member q-unix: bool 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' } }
"Non-array type name" makes no sense when talking about the QAPI schema,
because arrays don't have names there.
> 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
