Re: [Qemu-devel] [PATCH v10 01/25] tests/qapi-schema: Test for reserved names, empty struct

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@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 <eblake@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