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

[Eric Blake]

On 10/23/2015 06:33 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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/ ?
> 

Sounds better.

>> namespace, but here for commands as well as members.
> 
> This is reserved-member.json.

And reserve-commands.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 :)

No, the 'q_' paragraph covered both.  But yes, mentioning the actual
test names in the commit body can't hurt.

> 
>> 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 <address@hidden>
>>
>> ---

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

Probably true, but it is the current implementation, and we DO have a
compile time collision until (unless?) we bother to fix it.

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

Indeed - cleaning up command name mangling can come at a later date, and
maybe at that point we can decide 'q_' reservations for command names
doesn't add any power, and relax it at that point.

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

Suggestions? Maybe s/non-array//? The wording changes in 2/25 when the
test starts working.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature