[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions |
Date: |
Thu, 30 Jul 2015 17:44:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
- [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, (continued)
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Eric Blake, 2015/07/28
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Markus Armbruster, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Eric Blake, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Markus Armbruster, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Eric Blake, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Eric Blake, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Markus Armbruster, 2015/07/31
- Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions, Markus Armbruster, 2015/07/31
[Qemu-devel] [PATCH RFC v2 01/47] qapi: Clarify docs on including the same file multiple times, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 05/47] qapi: Reject -p arguments that break qapi-event.py, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 10/47] qapi-visit: Fix two name arguments passed to visitors, Markus Armbruster, 2015/07/01