From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58286) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSSgG-00079X-1y for qemu-devel@nongnu.org; Fri, 12 Sep 2014 11:22:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XSSg8-0005kM-KZ for qemu-devel@nongnu.org; Fri, 12 Sep 2014 11:22:36 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:52899) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSSg8-0005k5-F5 for qemu-devel@nongnu.org; Fri, 12 Sep 2014 11:22:28 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 12 Sep 2014 09:22:27 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <54125A84.6060503@redhat.com> References: <1410477659-9163-1-git-send-email-mdroth@linux.vnet.ibm.com> <1410477659-9163-2-git-send-email-mdroth@linux.vnet.ibm.com> <54125A84.6060503@redhat.com> Message-ID: <20140912152223.19243.41173@loki> Date: Fri, 12 Sep 2014 10:22:23 -0500 Subject: Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-stable@nongnu.org, armbru@redhat.com Quoting Eric Blake (2014-09-11 21:29:24) > On 09/11/2014 05:20 PM, Michael Roth wrote: > > In some cases an input visitor might bail out on filling out a > > struct for various reasons, such as missing fields when running > > in strict mode. In the case of a QAPI Union type, this may lead > > to cases where the .kind field which encodes the union type > > is uninitialized. Subsequently, other visitors, such as the > > dealloc visitor, may use this .kind value as if it were > > initialized, leading to assumptions about the union type which > > in this case may lead to segfaults. For example, freeing an > > integer value. > > = > = > > = > > +bool visit_start_union(Visitor *v, bool data_present, Error **errp) > > +{ > > + if (v->start_union) { > > + return v->start_union(v, data_present, errp); > > + } > > + return true; > > +} > = > Any rules on whether errp must be set if returning false, and must not > be set if returning true? If so, do we need a bool return, or is errp > sufficient? No restrictions in that regard, since this just an indicator of whether any work should be done on the union fields. For instance the dealloc visitor should continue operating normally and just skip over these rather than going into an error state. > = > = > > +++ b/scripts/qapi-visit.py > > @@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj= , const char *name, Error **e > > if (err) { > > goto out_obj; > > } > > + if (!visit_start_union(m, !!(*obj)->data, &err)) { > > + goto out_obj; > > + } > > switch ((*obj)->kind) { > = > and if there aren't rules, then a visitor that sets err but still > returns true would result in this code not exiting early, but passing an > already-set error into the switch, which is probably not desirable. Ahh, good catch. We should be jumping to out_obj: on error as well. > = > -- = > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org