Eric Blake <ebl...@redhat.com> 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.
