From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSDNa-0001og-7D for qemu-devel@nongnu.org; Thu, 11 Sep 2014 19:02:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XSDNR-0004bN-51 for qemu-devel@nongnu.org; Thu, 11 Sep 2014 19:02:18 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:52737) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSDNQ-0004bD-V4 for qemu-devel@nongnu.org; Thu, 11 Sep 2014 19:02:09 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Sep 2014 17:02:07 -0600 Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 1A0363E4003E for ; Thu, 11 Sep 2014 17:02:05 -0600 (MDT) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s8BN24NI50200806 for ; Fri, 12 Sep 2014 01:02:05 +0200 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s8BN24qS006186 for ; Thu, 11 Sep 2014 17:02:04 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <5411B34E.2080202@redhat.com> References: <1410352239-8705-1-git-send-email-famz@redhat.com> <54104BA9.4040308@redhat.com> <20140910150200.GA4883@fam-t430.nay.redhat.com> <54106F28.5080203@redhat.com> <20140911005328.GB2554@fam-t430.nay.redhat.com> <54112247.40303@redhat.com> <20140911043803.GA28795@fam-t430.nay.redhat.com> <20140911142649.32021.42205@loki> <5411B34E.2080202@redhat.com> Message-ID: <20140911230203.32021.16247@loki> Date: Thu, 11 Sep 2014 18:02:03 -0500 Subject: Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Fam Zheng , Eric Blake Cc: Kevin Wolf , Hu Tao , Markus Armbruster , qemu-devel@nongnu.org, Stefan Hajnoczi , Luiz Capitulino Quoting Paolo Bonzini (2014-09-11 09:35:58) > Il 11/09/2014 16:26, Michael Roth ha scritto: > > Also, the .kind field of a QAPI Union type is something we generate for= use > > by the generated visitor code. In the case of an unspecified discrimina= tor > > we generated the enum type for that field internally. In the case where= it's > > specified, we use an existing enum instead... > > = > > But nothing stops us from generating a new "shadow" enum in this case a= s well, > > with the indexes/integer values of the corresponding strings shifted by= one so > > we can reserve the 0 index for _INVALID. I think we can reasonably expe= ct that > > nothing outside the generated code makes use of those integer values in= this > > special case, and don't have to change all enum types to make that work. > = > But how would users fill in structs if you have to use a different enum? Argh, of course, we do still make direct use of these going in the other direction. Those users would need to use the "shadow" enum values to make it work, which is probably way too messy. > = > What about making adding visit_start_union/visit_end_union? > visit_start_union can return false if the visit of the union has to be > skipped. > = > The dealloc visitor can skip it if the data field is NULL; everything > else can just use a default implementation which always returns true. I forgot we had a void *data there as well. So we're basically relying on .data !=3D NULL implying that .kind has been properly initialized, rather than needing to encode anything into .kind... nice. I can imagine a case where we allocate memory for a set of union fields (so .data !=3D NULL) and then leave .kind uninitialized, which can still lead to segfaults due to improper casts in the dealloc visitor, but I don't really see a way around that. Even if we reserve .kind =3D=3D 0 for this purpose, it's still up to the user or visitor implementation to 0-initialize everything (though that's a bit easier to enforce). So this seems like a good approach. I've ahead and hacked something up which I'll send out shortly. > = > Paolo