Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/11/2014 08:42 AM, Markus Armbruster wrote:
>
>>>> Can anybody think of a use of c_var() that needs '.' preserved?
>>>
>>> Doing the replace in c_var() breaks some struct accesses in the generated
>>> code. I didn't look deeper to determine the users though.
>> 
>> Feels like a misuse of c_var() to me.
>> 
>> Dig, dig...  aha.  generate_visit_struct_fields() joins QAPI names
>> separated by '.', and passes the result to c_var().  I expect such code
>> to break when one of the names contains '.'.
>> 
>> It does indeed; try the appended patch to see it yourself.  It generates
>> 
>> struct VersionInfo
>> {
>>     struct 
>>     {
>>         int64_t major;
>>         int64_t minor;
>>         int64_t micro;
>>     } qemu;
>
> Wait a minute.  Isn't this one of the three cases of nested structs,
> where we were already arguing that nested structs are evil if we are
> going to introduce a fuller syntax for optional argument defaults?

Yes.

>>     struct 
>>     {
>>         int64_t major;
>>         int64_t minor;
>>         int64_t micro;
>>     } __com.redhat.crap;
>>     char *package;
>> };
>> 
>> Conclusion: this is simply a bug that needs fixing.
>> 
>> 
>> diff --git a/qapi/common.json b/qapi/common.json
>> index 4e9a21f..74ccde3 100644
>> --- a/qapi/common.json
>> +++ b/qapi/common.json
>> @@ -52,6 +52,7 @@
>>  ##
>>  { 'type': 'VersionInfo',
>>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
>> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 
>> 'int'},
>>             'package': 'str'} }
>
> And the fix may be as simple as ditching support for nested structs in
> the first place, and rewriting this as:
>
> { 'type': 'VersionDetails',
>   'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
> { 'type': 'VersionInfo',
>   'data': {'qemu': 'VersionDetails',
>            '__com.redhat.crap': 'VersionDetails',
>            'package': 'str' } }
>
> But the fact that we are still discussing makes it obvious - this is 2.2
> material.

Agree.  Let's ditch nested structs and see whether there are any misuses
of c_var() left.