From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avlYA-0005y7-Ab for qemu-devel@nongnu.org; Thu, 28 Apr 2016 09:00:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avlY9-0000wn-0r for qemu-devel@nongnu.org; Thu, 28 Apr 2016 09:00:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41728) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avlY8-0000wi-Mr for qemu-devel@nongnu.org; Thu, 28 Apr 2016 09:00:08 -0400 References: <1461801715-24307-1-git-send-email-eblake@redhat.com> <1461801715-24307-3-git-send-email-eblake@redhat.com> <87pot9or8r.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <57220957.1050608@redhat.com> Date: Thu, 28 Apr 2016 07:00:07 -0600 MIME-Version: 1.0 In-Reply-To: <87pot9or8r.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bCBcIMP24iGil81sK7H3f9mVTeOTSC8rT" Subject: Re: [Qemu-devel] [PATCH v15 02/23] qapi: Guarantee NULL obj on input visitor callback error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --bCBcIMP24iGil81sK7H3f9mVTeOTSC8rT Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/28/2016 06:24 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Our existing input visitors were not very consistent on errors >> in a function taking 'TYPE **obj' (that is, start_struct(), >> start_alternate(), next_list(), type_str(), and type_any()). >> While all of them set '*obj' to allocated storage on success, >> it was not obvious whether '*obj' was guaranteed safe on failure, >> or whether it was left uninitialized. But a future patch wants >> to guarantee that visit_type_FOO() does not leak a partially- >> constructed obj back to the caller; it is easier to implement >> this if we can reliably state that '*obj' is assigned on exit, >> even on failures. Add assertions to enforce it. >=20 > I had to read this several times, because by now I've forgotten that > we're talking about input visitors only. Easy enough to avoid: ... tha= t > input visitors assign to *obj regardless of success or failure. >=20 > Begs the question what is assigned to it on failure, though. Easy enough to improve the commit message - the intent is that these all set *obj to NULL on failure. >> + v->start_struct(v, name, obj, size, &err); >> + if (obj && v->type =3D=3D VISITOR_INPUT) { >> + assert(err || *obj); >> + } >> + error_propagate(errp, err); >> } >=20 > The commit message claims you're adding assertions to enforce input > visitors assign *obj even on failure. This assertion doesn't do that. > It enforces "on success, *obj is non-null". Is that what you want? Or= > do you actually want something like "either err or *obj are non-null"? > I.e. >=20 > assert(!err !=3D !*obj); Hmm - that's an even tighter assertion. I'll run it through the testsuite, and if it passes, I'll use it. >=20 > Hmm, you check the postcondition only when v implements > start_alternate(). Shouldn't it hold regardless of v? If yes, then > let's check it regardless of v: >=20 > if (v->start_alternate) { > v->start_alternate(v, name, obj, size, promote_int, &err); > } > if (v->type =3D=3D VISITOR_INPUT) { > assert(err || *obj); > } > error_propagate(errp, err); >=20 > But that makes it pretty obvious that the postcondition won't hold when= > !v->start_alternate. May v->start_alternate() be null for an input > visitor? According to visitor-impl.h, it may not. Okay. Doesn't hurt to make the check unconditional (to make sure no new input visitors forget to implement start_alternate). I'm also debating about adding an assertion (more likely in the doc patch, since it is less related to the theme of this one) that obj->type is sensible. >=20 > The commit message lists start_struct(), start_alternate(), next_list()= , > type_str(), and type_any(). You cover them except for next_list(). Wh= y > is that missing? Because *obj can be NULL after next_list() if the list is empty. But there may still be a weaker assertion worth having: if err, then *obj must be NULL; and if *obj, then err must not be set (weaker in that for all the other functions touched, exactly one of the two conditions can result, but here, !err and !*obj is valid as a third condition). Depending on what else you find later in the series, I may just post a fixup for this patch. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --bCBcIMP24iGil81sK7H3f9mVTeOTSC8rT Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXIglXAAoJEKeha0olJ0NqzdMIAJzMtn2umQrXShvjuGyIAcok 38hRCtVQq/gtRSK4NRREobEGWLPW41rb8ojGdiYtLyCMBF4smYtDDyoCxt/XT1Iw FILvA0imI6XD9Z4cLWsCDu4ofr+0Dl0Rz8bFTpcjR974Uzi61wS3wc7XYsY5kUIf Jf5uZK6xNoyPGxnV/JDPbXIyG5dDvHRH1sXa4STZ07TxJKibkkF0D0giFxLci2kt ifR1tBwaxUeNXEI92rPPn/Q8x2z3cfzWlhlWGVHCgay4F8pCee4jdTmKVeL3GhdQ UztxDX1E+KtvD4SKbaUacm/mxoyKOLKmofcGmFmi9tNSAZjwGu3gH5FHo2DK/rE= =qrgz -----END PGP SIGNATURE----- --bCBcIMP24iGil81sK7H3f9mVTeOTSC8rT--