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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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'?

Good questions.  Someone will have to review and fix this code.  Let's
add a FIXME so we don't forget.  Care to propose one?

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

Either we reject empty unions, or we detect them and add a dummy,
similar to what we do for structs since commit 83ecb22.  I figure simply
rejecting them is easier.