From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33081) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuFMu-0007Zk-Jk for qemu-devel@nongnu.org; Thu, 05 Nov 2015 02:54:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuFMp-00045r-FH for qemu-devel@nongnu.org; Thu, 05 Nov 2015 02:54:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33440) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuFMp-00045i-7Y for qemu-devel@nongnu.org; Thu, 05 Nov 2015 02:53:55 -0500 From: Markus Armbruster 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> <563A7328.2050807@redhat.com> Date: Thu, 05 Nov 2015 08:53:51 +0100 In-Reply-To: <563A7328.2050807@redhat.com> (Eric Blake's message of "Wed, 4 Nov 2015 14:05:44 -0700") Message-ID: <874mh07ugg.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > On 11/04/2015 01:40 AM, Markus Armbruster wrote: > >> >>> 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. >> >> This part isn't as obvious. >> >> 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(TestInputVisitorData *data, >>> /* Parsing an int */ >>> >>> v = visitor_input_test_init(data, "42"); >>> - visit_type_AltStrBool(v, &asb, NULL, &err); >>> - g_assert(err); >>> - error_free(err); >>> - err = NULL; >>> + visit_type_AltStrBool(v, &asb, NULL, &data->err); >>> + g_assert(data->err); >>> qapi_free_AltStrBool(asb); >> >> This leaves data->err non-null. >> >>> >>> /* FIXME: Order of alternate should not affect semantics; asn should >>> * parse the same as ans */ >>> v = 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); >> >> 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. You're right. >> If it fails to error out, its error_propagate() does nothing, and we >> won't detect the test failure. >> >> 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. >> >> Is it a good idea anyway? Let's discuss before I check the remainder of >> 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. Reducing boilerplate is good. However, I'm afraid hiding error_free(err); err = NULL; like this sets an example that is easily copied incorrectly. Here's what error.h has to say about consuming an Error object: * Report an error to stderr: * error_report_err(err); * This frees the error object. * * Report an error somewhere else: * const char *msg = error_get_pretty(err); * do with msg what needs to be done... * error_free(err); * * Handle an error without reporting it (just for completeness): * error_free(err); Perhaps we want something like * Expect an error, abort() if there is none: * error_free_or_abort(&err); * This frees the error object and clears err. Convenient for tests. Then the patch quoted above becomes @@ -364,21 +341,17 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, /* Parsing an int */ v = visitor_input_test_init(data, "42"); visit_type_AltStrBool(v, &asb, NULL, &err); - g_assert(err); - error_free(err); - err = NULL; + error_free_or_abort(&err); qapi_free_AltStrBool(asb); /* FIXME: Order of alternate should not affect semantics; asn should * parse the same as ans */ v = visitor_input_test_init(data, "42"); visit_type_AltStrNum(v, &asn, NULL, &err); /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */ /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */ - g_assert(err); - error_free(err); - err = NULL; + error_free_or_abort(&err); qapi_free_AltStrNum(asn); v = visitor_input_test_init(data, "42"); Similar boilerplate reduction, but keeps the clearing of err more visible.