From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55117) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zujoc-00087m-KL for qemu-devel@nongnu.org; Fri, 06 Nov 2015 11:24:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZujoZ-0002He-8t for qemu-devel@nongnu.org; Fri, 06 Nov 2015 11:24:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35334) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZujoZ-0002Ha-4U for qemu-devel@nongnu.org; Fri, 06 Nov 2015 11:24:35 -0500 From: Markus Armbruster 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> <563CCD2F.5090703@redhat.com> Date: Fri, 06 Nov 2015 17:24:32 +0100 In-Reply-To: <563CCD2F.5090703@redhat.com> (Eric Blake's message of "Fri, 6 Nov 2015 08:54:23 -0700") Message-ID: <87bnb7w0xr.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > On 11/06/2015 08:36 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> 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 = pt->type; >>> - ops->serialize(pt, &serialize_data, visit_primitive_type, &err); >>> - ops->deserialize((void **)&pt_copy, serialize_data, >>> visit_primitive_type, &err); >>> + ops->serialize(pt, &serialize_data, visit_primitive_type, &error_abort); >>> + ops->deserialize((void **)&pt_copy, serialize_data, >>> visit_primitive_type, >>> + &error_abort); >>> >>> - g_assert(err == NULL); >> >> This looks like a (very minor) bug fix / cleanup: you're not supposed to >> pass the same &err to multiple functions without checking and clearing >> it in between, because the second failure trips assert(*errp == NULL) in >> error_setv(). Harmless here, but it's nice to get rid of a bad example. > > 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. > >> >> 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. Works for me.