From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36587) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZujLS-00013G-Ov for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:54:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZujLN-0000LV-R0 for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:54:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54442) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZujLN-0000Kl-JG for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:54:25 -0500 References: <1446791754-23823-1-git-send-email-eblake@redhat.com> <1446791754-23823-7-git-send-email-eblake@redhat.com> <87611f16nk.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <563CCD2F.5090703@redhat.com> Date: Fri, 6 Nov 2015 08:54:23 -0700 MIME-Version: 1.0 In-Reply-To: <87611f16nk.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KWm0ccWAp4BW8hqQwcHjg7c1lA9j8MOjd" Subject: Re: [Qemu-devel] [PATCH v10 06/30] qapi: Simplify non-error testing in test-qmp-* 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) --KWm0ccWAp4BW8hqQwcHjg7c1lA9j8MOjd Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/06/2015 08:36 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> By using &error_abort, we can avoid a local err variable in >> situations where we expect success. It also has the nice >> effect that if the test breaks, the error message from >> error_abort tends to be nicer than that of g_assert(). >> >> Signed-off-by: Eric Blake > [Boring mechanical changes snipped...] >> pt_copy->type =3D pt->type; >> - ops->serialize(pt, &serialize_data, visit_primitive_type, &err); >> - ops->deserialize((void **)&pt_copy, serialize_data, visit_primiti= ve_type, &err); >> + ops->serialize(pt, &serialize_data, visit_primitive_type, &error_= abort); >> + ops->deserialize((void **)&pt_copy, serialize_data, visit_primiti= ve_type, >> + &error_abort); >> >> - g_assert(err =3D=3D NULL); >=20 > This looks like a (very minor) bug fix / cleanup: you're not supposed t= o > pass the same &err to multiple functions without checking and clearing > it in between, because the second failure trips assert(*errp =3D=3D NUL= L) in > error_setv(). Harmless here, but it's nice to get rid of a bad example= =2E Harmless here because we are asserting that err is still NULL after the chain (which means it was NULL at all points during the chain). But I agree that it is nice to get rid of poor practice, and that adding a paragraph to the commit message to point it out would be a nice idea. >=20 > Suggest to note the cleanup in the commit message. We may be close enough to take the series without needing a v11; if that's the case, and you are the one squashing in the change, how about this text: This patch has an additional bonus of fixing several call sites that were passing &err to two different functions without checking it in between. In general that is unsafe practice; because if the first function sets an error, the second function could abort() if it tries to set a different error. We got away with it because we were asserting that err was NULL through the entire chain, but switching to &error_abort avoids the questionable practice up front. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --KWm0ccWAp4BW8hqQwcHjg7c1lA9j8MOjd 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/ iQEcBAEBCAAGBQJWPM0vAAoJEKeha0olJ0NqMY8IAJxhbO6gcOE6DhoeJhAKdLan K2HZRArGbZvd1rTCJJXIzBFOVjTHKTfkefBh96/cyxSg+0RhD4QVQG3G1wtWyiYK ywBPYg1vMTJU5sDQvYQ2iwEcsy0jTpJk56rfwBQ0uu3Dw1Yd6HmpqCMqfZN6Y91F 8X1Y4VyzPhKDNGb4pqndj9U4TyipqXKhXqqSDn3eF4tJU4TgfGbJ2yfvza7hIjTO z9tTb9eBD2EEH1MypG/HPJmP3LRo6hXdUl3fUZPCDpjKoumJHl5SCFgHF/qlGs7d C5yMXOoLnT8qNa5OnDuUkh0f7xjYVhYTj3X/yNFIYfgGArevPE3Jmxq8E4OdBrk= =pR6F -----END PGP SIGNATURE----- --KWm0ccWAp4BW8hqQwcHjg7c1lA9j8MOjd--