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

[Eric Blake]

On 10/19/2015 10:05 AM, Markus Armbruster wrote:
> 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 :)

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