From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36048) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKq0z-0000rc-8g for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:45:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKq0u-0002P5-9I for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:45:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33742) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKq0u-0002Ox-48 for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:44:56 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-8-git-send-email-armbru@redhat.com> <55B7E15D.50902@redhat.com> <87lhdzju4i.fsf@blackfin.pond.sub.org> <55B93445.9060104@redhat.com> <87io923ysb.fsf@blackfin.pond.sub.org> <55BA3157.2070806@redhat.com> Date: Thu, 30 Jul 2015 17:44:51 +0200 In-Reply-To: <55BA3157.2070806@redhat.com> (Eric Blake's message of "Thu, 30 Jul 2015 08:14:47 -0600") Message-ID: <87zj2dy7ik.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake 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.