From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45444) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu5Fi-0001XF-Ds for qemu-devel@nongnu.org; Wed, 04 Nov 2015 16:05:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zu5Ff-00020t-6p for qemu-devel@nongnu.org; Wed, 04 Nov 2015 16:05:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57974) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zu5Fe-00020n-RC for qemu-devel@nongnu.org; Wed, 04 Nov 2015 16:05:51 -0500 References: <1446618049-13596-1-git-send-email-eblake@redhat.com> <1446618049-13596-5-git-send-email-eblake@redhat.com> <87mvuutaxo.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <563A7328.2050807@redhat.com> Date: Wed, 4 Nov 2015 14:05:44 -0700 MIME-Version: 1.0 In-Reply-To: <87mvuutaxo.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QaA6dIDR0PkaKJ0VM9fb7Nmc2vkGBEWlf" Subject: Re: [Qemu-devel] [PATCH v9 04/27] qapi: Simplify 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) --QaA6dIDR0PkaKJ0VM9fb7Nmc2vkGBEWlf Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/04/2015 01:40 AM, Markus Armbruster wrote: >=20 >> By moving err into data, we can let test teardown take care >> of cleaning up any collected error; it also gives us fewer >> lines of code between repeated tests where init runs teardown >> on our behalf. >=20 > This part isn't as obvious. >=20 > Having two parts of differing obviousness indicates patch splitting > could make sense. Especially when the parts are large and mechanical, > because reviewing large mechanical changes is much easier when there's > just one kind of it. Will split. >> @@ -364,21 +341,17 @@ static void test_visitor_in_alternate_number(Tes= tInputVisitorData *data, >> /* Parsing an int */ >> >> v =3D visitor_input_test_init(data, "42"); >> - visit_type_AltStrBool(v, &asb, NULL, &err); >> - g_assert(err); >> - error_free(err); >> - err =3D NULL; >> + visit_type_AltStrBool(v, &asb, NULL, &data->err); >> + g_assert(data->err); >> qapi_free_AltStrBool(asb); >=20 > This leaves data->err non-null. >=20 >> >> /* FIXME: Order of alternate should not affect semantics; asn sho= uld >> * parse the same as ans */ >> v =3D visitor_input_test_init(data, "42"); And this part wipes out data, so that data->err is once again NULL. >> - visit_type_AltStrNum(v, &asn, NULL, &err); >> + visit_type_AltStrNum(v, &asn, NULL, &data->err); >=20 > If visit_type_AltStrNum() errors out, its error will be thrown away by > its error_propagate(), because data->err is already non-null. No, you missed that this patch is relying on the magic in 3/27 that wipes out data on every new test. >=20 > If it fails to error out, its error_propagate() does nothing, and we > won't detect the test failure. >=20 > Your patch makes forgetting to reset err an even easier mistake to make= , > because it removes most of the error_free() that serve as reminders. >=20 > Is it a good idea anyway? Let's discuss before I check the remainder o= f > this patch. The point was that by moving err _into_ data, then the resetting of err becomes as simple as resetting data, rather than having to be verbose at every use of data->err. We just need a visitor_input_test_init() in between. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --QaA6dIDR0PkaKJ0VM9fb7Nmc2vkGBEWlf 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/ iQEcBAEBCAAGBQJWOnMoAAoJEKeha0olJ0NqTOEIAInJqLwEe8Oz9/YAtE86srmD lDLx4kZat4OmOw9VQAjtS7Hgz6WKls8fKHQazC9jF2vy+k75tJEcjMLXpKhlyldd KQ1nrs0Alp7Q2q/QBTBnf8oYXS+JpIfQx0WYL9c/ON3V+/322/Vhbd1OYOE8I6vr CXDa4DFT3JIv1nB+Z6VjPl07IQvIIN4iw1eJPv11asDOxF/dPvNI0OpMTn2jYKjY jZPtkmhFLO+HosrEiNBfMlBU1oJ7nq5SrEpiFSPtJU3fTLFxrFynuwEcyrZy+/h7 rRM7EOeidR7+gbbr4NqDkSiWXNOc5eZR7+lnI6xSl7wWBuhVQr4QyUhyBTdDbcQ= =EIPb -----END PGP SIGNATURE----- --QaA6dIDR0PkaKJ0VM9fb7Nmc2vkGBEWlf--