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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.)

This part is awkward: we're adding a negative test that fails to fail
for use of a name that isn't actually reserved until two patches later.

I guess I'd move the 'u' tests to the patch that reserves the name.  Or
at least start the commit message with one of the non-awkward cases :)

> 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.

I started to write that you might want to mention we already reserve the
'Kind' suffix, then noticed you do in PATCH 02.  No need to say it
twice.

> 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.

However, we could *choose* to enforce a single name space for
simplicity.  By convention, type names are StudlyCaps (except for
built-ins), and member names are dashed-lower-case, so collisions are
unlikely anyway.

Perhaps you should write "there is no technical reason".

> 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(-)

Since this is a test-only patch, I'd prefix the subject with
"tests/qapi-schema:" instead of "qapi:".

>  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' } }

Complements existing args-name-clash.json, which tests 'a-b' and 'a_b'.

Call it args-has-clash.json?

> 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' } }

This replaces the test of branch name 'c-d' clashing with member 'c_d'
by a test of branch name 'has-a' clashing with the has_a flag of
optional member 'a'.  Okay, since flat-union-clash-type.json covers
clash of branch name with member name.

> 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'} }

Value of these positive tests seems marginal.  But if you think they're
worth keeping, I'll take them.

> 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' } }

If the later patch outlaws 'u' in structs as well, this test will do,
only the comment will change.

If it outlaws 'u' only where it actually clashes, namely in unions, the
test will need updating.

More reason to move the test to the patch that does the outlawing.

> 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