Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions

[Eric Blake]

On 07/30/2015 01:11 AM, Markus Armbruster wrote:

>> Another name collision bug: our code generates flat unions as:
>>
>> struct BlockdevOptions {
>>     BlockdevDriver driver;
>> ...
>>     /* End fields inherited from BlockdevOptionsBase. */
>>     /* union tag is BlockdevDriver driver */
>>     union {
>>         void *data;
>>         BlockdevOptionsArchipelago *archipelago;
>> ...
>>
>> which means that if we name any of the branches 'data' (that is, if
>> 'data' is a member of the enum discriminator), things fail to compile.
>> We could probably fix that by naming our dummy branch '_data'.
> 
> I wonder whether member data is actually used.  I'll find out.

The dealloc visitor uses 'data' being non-null as a flag on whether to
deallocate the union even if the tag was invalid for some reason; or
more importantly, if parsing consumed the tag but then detected an error
while parsing the union, leaving the union branch partially allocated.
To avoid a leak, we have to deallocate the branch.

But if the tag was invalid, then why did we ever allocate the union in
the first place, and how do we prove we are calling the correct free-ing
function?  And if the tag is valid, why can't we just guarantee that the
union is 0-initialized and that deleting the branch will work through
the correct branch type instead of worrying about 'data'?

We still need a dummy member if it is valid to do { 'union':'Foo',
'data':{} } since C doesn't like empty unions, but an empty union seems
like something we may want to reject, at which point you are probably
right that deleting the data member altogether should work and still let
us recover from bad partial parses without a leak.

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

signature.asc
Description: OpenPGP digital signature