From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54403) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgUfm-0007ug-CF for qemu-devel@nongnu.org; Mon, 28 Sep 2015 05:24:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZgUfh-00009c-RU for qemu-devel@nongnu.org; Mon, 28 Sep 2015 05:24:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZgUfh-00009E-KN for qemu-devel@nongnu.org; Mon, 28 Sep 2015 05:24:33 -0400 From: Markus Armbruster References: <1442872682-6523-1-git-send-email-eblake@redhat.com> <1442872682-6523-8-git-send-email-eblake@redhat.com> <87r3lneuet.fsf@blackfin.pond.sub.org> <560710F4.40003@redhat.com> Date: Mon, 28 Sep 2015 11:24:30 +0200 In-Reply-To: <560710F4.40003@redhat.com> (Eric Blake's message of "Sat, 26 Sep 2015 15:41:08 -0600") Message-ID: <87vbaukib5.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Michael Roth , marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, DirtY.iCE.hu@gmail.com Eric Blake writes: > On 09/24/2015 08:58 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> Due to the existing semantics of the error_set() family, >>> generated sequences in the qapi visitors such as: >>> >>> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >>> if (!err) { >>> visit_type_FOO_fields(m, obj, errp); >>> visit_end_implicit_struct(m, &err); >>> } >>> error_propagate(errp, err); >> >> Indentation seems off. Intentional? >> >>> >>> are risky: if visit_type_FOO_fields() sets errp, and then >>> visit_end_implicit_struct() also encounters an error, the >>> attempt to overwrite the first error will cause an abort(). > > I didn't even read error_propagate()'s contract correctly. It > specifically specifies that if errp is already set, then err is ignored. Yes. Differs from error_set() & friends, where the destination must not contain an error. The inconsistency is a bit ugly. Perhaps it adds enough convenience to make it worthwhile. Anyway, changing it now would be a huge bother. Note that GLib's g_propagate_error() requires the destination not to contain an error. > So the above sequence is actually just fine, because only the following > paths exist: > > visit_start_implicit_struct() fails into &err, > error_propagate() passes err into caller's errp > > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() fails into caller's errp, > visit_end_implicit_struct() succeeds, > error_propagate() does nothing > > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() fails into caller's errp, > visit_end_implicit_struct() fails int &err, > error_propagate() does nothing (errp trumps err) Yes, but visit_end_implicit_struct() gets called with an errp argument that may already contain an error, and that's unusual. Prominent notice in the contract required. > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() succeeds, > visit_end_implicit_struct() fails int &err, > error_propagate() passes err into caller's errp > > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() succeeds, > visit_end_implicit_struct() succeeds, > error_propagate() does nothing > > > As such, I'm revisiting if anything is needed at all, other than making > the various visit_start/visit_end patterns consistent with one another > using existing idioms, and it may turn out we don't need the ternary > after all.