From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41268) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKTTl-0005VT-Tt for qemu-devel@nongnu.org; Wed, 29 Jul 2015 11:41:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKTTg-0003pY-U1 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 11:41:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48928) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKTTg-0003p8-N0 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 11:41:08 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-30-git-send-email-armbru@redhat.com> <55B025B9.2060004@redhat.com> <87zj2gohv1.fsf@blackfin.pond.sub.org> <55B79754.6090609@redhat.com> <87y4hzicto.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <55B8F40E.9020301@redhat.com> Date: Wed, 29 Jul 2015 09:41:02 -0600 MIME-Version: 1.0 In-Reply-To: <87y4hzicto.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="oUweCtnKl1162TNCcInkQXToBCPcINEkI" Subject: Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --oUweCtnKl1162TNCcInkQXToBCPcINEkI Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/29/2015 02:32 AM, Markus Armbruster wrote: >>> 2. We can leak retval only when qmp_FOO() returns non-null and local_= err >>> is non-null. This must not happen, because: >>> >>> a. local_err must be null before the call, and >>> >>> b. the call must not return non-null when it sets local_err. >> >> We don't state that contract anywhere, but I doubt any of the qmp_FOO(= ) >> functions violate it, so it is worth making it part of the contract. >=20 > It's a general Error API rule: set an error exactly on failure. It > applies to any function returning errors through an Error **errp > parameter, and we generally don't bother to spell it out for the > individual functions. >=20 > The part that needs to be spelling out is what success and failure mean= =2E > A qmp_FOO() returning an object returns null on failure. >=20 >>> We could right after out: assert(!local_err || !retval). Not sure= >>> it's worthwhile. >> >> I think it IS worthwhile, because it would catch buggy callers. Not >=20 > We use the same assumption all over the place, without asserting it. Okay, you've got a point there. >=20 > Let's drop the useless initializer. As explained above, I don't like > the assertion for reasons explained above, but if you want it, I'm > willing to take it anyway, in a separate follow-up patch. >=20 > I'd prefer to drop the initializer myself myself (with you credited in > the commit message), because it's certainly less total work, and quite > possibly less work for just for me. No need to add assertions. Maybe worth a patch to add a comment somewhere, maybe as a new section in docs/qapi-code-gen.txt, documenting how to write a handler and hook it into qmp-commands.hx, and what conditions the handler must obey. And I'm fine with you dropping the initializer as part of your v3 series. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --oUweCtnKl1162TNCcInkQXToBCPcINEkI 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/ iQEcBAEBCAAGBQJVuPQSAAoJEKeha0olJ0NqyUMH/0lWJ3kmJZ+Z9IEtoguqMNDn j8VerxOHbnOOrrQ8si0cuwyPdqax94nkn3wfh1YeIopkuAmiRC2EOYGeL1SPiRQG /EVSYPra1woMpTpA5sogAGpGCn/nLi8jqjN/NMllelU0OAQli428kJA9udT/WXb9 fIzW2NG7BBkK+tQcC4rSo3hGPkP3hVdRdaNMdy5aMoQtcOee5Bu+jbxowIDibjUY QL2vp2YaECKmYsdQNmg3fQLOP7ZfIjeKMk+SaCWXpj6UvVzrP9Isg3/E7I1NAxyv XJQJgGHFIwbpQwdARFoaq6OiTPpN5d6oPGF+tX9L61yHnYxWJtKbA7MU9wqLMOw= =xKJB -----END PGP SIGNATURE----- --oUweCtnKl1162TNCcInkQXToBCPcINEkI--