On 10/19/2015 10:05 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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 :)
If you are okay with it, I can squash this test into commits 2 and 3, so
that we aren't introducing tests and changing state later, but instead
just adding tests at the time I reserve namespace.
> 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.
Especially if I take the approach of not having a separate patch 1 that
just adds tests.
Sometimes, I find it easier to add tests showing one behavior, then flip
the switch to show how the new behavior changes the behavior. But I can
be persuaded that it is just churn, and that adding the new tests at the
same time as the new behavior is just as effective. :)
>
>> 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".
Well, I _do_ have a possible technical reason in mind: we've already
mentioned that we may want to someday support automatic '-'/'_' munging;
and it is not too much of a further jump to support case-insensitive
matching (at least on a US keyboard, - and _ are on the same key the
same, so it is already akin to a case conversion to go between the two).
With case-insensitive member names, we then have collisions between a
MyType type name and a mytype member (where the QMP client can still
access the member via MyType). But yeah, I can revisit the wording of
this paragraph.
> Since this is a test-only patch, I'd prefix the subject with
> "tests/qapi-schema:" instead of "qapi:".
Unless I split and squash it with other patches, of course.
>> +++ 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?
Sure, can do.
>> +++ 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.
And since the test disappears completely later in my pending work, once
I change the layout to use a named union. (See subset C 6/14
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04111.html)
>> +# 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.
I added even more of these positive tests in subset C. And then I
noticed that you've made the same comment about the positive test I
added in patch 4/17. I can go either way (the added positive tests
helped me ensure that things compiled while writing patches, but I won't
be offended to omit them if you don't think they add enough to keep
long-term).
>> +++ 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.
Fair enough. And yes, I outlawed 'u' everywhere, not just in unions, on
the grounds that it is possible to convert a struct to a union while
remaining backwards-compatible in the QMP that it accepts; the act of
converting between object types must not invalidate an existing object
member, so if we reserve a member name, we should reserve it for all
objects and not just qapi unions.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
