From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zyhuk-00084S-8z for qemu-devel@nongnu.org; Tue, 17 Nov 2015 10:11:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zyhug-0005Jy-MP for qemu-devel@nongnu.org; Tue, 17 Nov 2015 10:11:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36366) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zyhug-0005J0-E9 for qemu-devel@nongnu.org; Tue, 17 Nov 2015 10:11:18 -0500 References: <1447352454-29349-1-git-send-email-eblake@redhat.com> <87egfospna.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <564B4391.50304@redhat.com> Date: Tue, 17 Nov 2015 08:11:13 -0700 MIME-Version: 1.0 In-Reply-To: <87egfospna.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UHWvbvgO4Q4koNv0vtv7gj06aFUj3kELe" Subject: Re: [Qemu-devel] [PATCH for-2.5] error: Document chained error handling 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) --UHWvbvgO4Q4koNv0vtv7gj06aFUj3kELe Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/17/2015 06:48 AM, Markus Armbruster wrote: >> --- >> based on feedback of my qapi series v5 7/46; doc only, so might >> be worth having in 2.5 >> --- >> + * >> + * In a situation where cleanup must happen even if a first step fail= s, >> + * but the cleanup may also set an error, the first error to occur wi= ll >> + * take priority when combined by: >> + * Error *err =3D NULL; >> + * action1(arg, errp); >> + * action2(arg, &err); >> + * error_propagate(errp, err); Your proposal covers this idiom. >> + * or by: >> + * Error *err =3D NULL; >> + * action1(arg, &err); >> + * action2(arg, err ? NULL : &err); >> + * error_propagate(errp, err); This idiom doesn't appear in the current code base, so not documenting it is okay... >> + * although the second form is required if any further error handling= >> + * will inspect err to see if all earlier locations succeeded. =2E..if we instead document how to check if either error happened, but your version also does that. >> */ >> >> #ifndef ERROR_H >=20 > Yet another one: >=20 > * Error *err =3D NULL, *local_err =3D NULL; > * action1(arg, &err); > * action2(arg, &local_err) > * error_propagate(&err, err); This line should be error_propagate(&err, local_err); > * error_propagate(errp, err); >=20 > Less clever. >=20 > Can we find a single, recommended way to do this? See below. >=20 > Not mentioned: the obvious >=20 > action1(arg, errp); > action2(arg, errp); >=20 > is wrong, because a non-null Error ** argument must point to a null. I= t > isn't when errp is non-null, and action1() sets an error. It actually > fails an assertion in error_setv() when action2() sets an error other > than with error_propagate(). Indeed, pointing out what we must NOT do is worthwhile. >=20 > The existing how-to comment doesn't spell this out. It always shows th= e > required err =3D NULL, though. You can derive "must point to null" fro= m > the function comments of error_setg() and error_propagate(). >=20 > I agree the how-to comment could use a section on accumulating errors. > Your patch adds one on "accumulate and pass to caller". Here's my > attempt: >=20 > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 4d42cdc..b2362a5 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -76,6 +76,23 @@ > * But when all you do with the error is pass it on, please use > * foo(arg, errp); > * for readability. > + * > + * Receive and accumulate multiple errors (first one wins): > + * Error *err =3D NULL, *local_err =3D NULL; > + * foo(arg, &err); > + * bar(arg, &local_err); > + * error_propagate(&err, local_err); > + * if (err) { > + * handle the error... > + * } > + * > + * Do *not* "optimize" this to > + * foo(arg, &err); > + * bar(arg, &err); // WRONG! > + * if (err) { > + * handle the error... > + * } > + * because this may pass a non-null err to bar(). I like that. > */ > =20 > #ifndef ERROR_H >=20 > Leaves replacing &err by errp when the value of err isn't needed to the= > reader. I think that's okay given we've shown it already above. >=20 > What do you think? I agree; knowing when it is safe to replace &err by errp is already sufficiently covered in existing text, and limiting this example to a single point is better. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --UHWvbvgO4Q4koNv0vtv7gj06aFUj3kELe 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/ iQEcBAEBCAAGBQJWS0ORAAoJEKeha0olJ0NqTIMIAICicGihSKTJCf1uQfWv7F3G puGRAxhv6HNlIPwjFpGuWXzyDqgX6NfXWdLEOfpmrR/7QBP1UDR1mL6tlWke50qt wGuhj9jbdHbfc4SOo3nQXt9yuEAtE66B0Rd8nusr0ANg1SL1jHK9qFE+/Xj38S7k XC+91FN+lzDhf+ZqbeYgY8LGfOlxSdnYuXgEEddqKBMcg/tCOmzCxt2FJH4T+bp1 n3VA6KyWPsLG3Jol1OTiCCknsiklkbSuPhxH1aPiwA6GtSAeOI5+3FVFxzXPSJlN J/dBMV0SGHOGXvgIqnBxWekwG3fdKVCMuyS8LmuPpfiyGuNjYECA8j3fwJ4/9fM= =QI9i -----END PGP SIGNATURE----- --UHWvbvgO4Q4koNv0vtv7gj06aFUj3kELe--