From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47579) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS5Ky-0003UP-Dj for qemu-devel@nongnu.org; Thu, 11 Sep 2014 10:27:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XS5Kq-0001LS-Kp for qemu-devel@nongnu.org; Thu, 11 Sep 2014 10:27:04 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:39212) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS5Kq-0001LK-Ed for qemu-devel@nongnu.org; Thu, 11 Sep 2014 10:26:56 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Sep 2014 08:26:55 -0600 Received: from b03cxnp08028.gho.boulder.ibm.com (b03cxnp08028.gho.boulder.ibm.com [9.17.130.20]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 2DB7B1FF003F for ; Thu, 11 Sep 2014 08:26:51 -0600 (MDT) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by b03cxnp08028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s8BEQqZj21430500 for ; Thu, 11 Sep 2014 16:26:52 +0200 Received: from d03av01.boulder.ibm.com (localhost [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s8BEQp8u021718 for ; Thu, 11 Sep 2014 08:26:52 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20140911043803.GA28795@fam-t430.nay.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> Message-ID: <20140911142649.32021.42205@loki> Date: Thu, 11 Sep 2014 09:26:49 -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: Fam Zheng , Eric Blake Cc: Kevin Wolf , Hu Tao , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini , Luiz Capitulino Quoting Fam Zheng (2014-09-10 23:38:03) > On Wed, 09/10 22:17, Eric Blake wrote: > > On 09/10/2014 06:53 PM, Fam Zheng wrote: > > > On Wed, 09/10 17:32, Paolo Bonzini wrote: > > >> Il 10/09/2014 17:02, Fam Zheng ha scritto: > > >>>> A bit hackish, but I don't have any better idea. > > >>>> > > >>>> Hmm... what about adding a new member to the visitors for "invalid= enum" > > >>>> value? The dealloc visitor could override it to do nothing, while= the > > >>>> default could abort or set an error. Would that work? > > >>> > > >>> The invalid state of enum still needs to be saved in the data. It = is detected > > >>> by the input visitor, but should be checked by other visitors (outp= ut, dealloc) > > >>> later. > > >> > > >> Yes, that's fine. The only part where I'm not sure is the special > > >> casing of the _MAX enum. > > >> > > > = > > > Yes, it is abusing. Let's add an _INVALID =3D 0 enum which is much cl= earer. > > = > > If I understand correctly, you mean that for: > > = > > { 'enum': 'Foo', 'data': [ 'one', 'two' ] } > > = > > FOO_ONE would now be 1 instead of its current value of 0? > > = > > We just barely saw a case where Hu Tao's code was relying on the > > implicit value 0 assigned to the first enum in the json file [1] > > although I strongly argued that it should be nuked (and so it was fixed > > in [2]). So I could live with reserving 0 for internal use for flagging > > parse errors (such as attempting to pass the string 'three' where a Foo > > value is expected). > > = > > [1] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01691.html > > [2] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01938.html > > = > = > A closer looking shows me a huge risk with _lookup table. We have for loo= ps > starting with index 0 all over the place to peek info _lookup arrays, but= in > this case it is _INVALID and shouldn't be looked at. If we fix up the generator code to fill the 0 entry with a "_qapi_invalid" string or something of the like then we shouldn't trigger any issues iterat= ing the lookup arrays. 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 discriminator 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 as we= ll, 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 expect t= hat 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. > = > Maybe we have to put _INVALID at the end after _MAX. > = > Fam