[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()



On 7/7/20 11:50 AM, Markus Armbruster wrote:
From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>

Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal and error_prepend/error_append_hint: user

the user

can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
Reviewed-by: Paul Durrant <paul@xxxxxxx>
Reviewed-by: Greg Kurz <groug@xxxxxxxx>
Reviewed-by: Eric Blake <eblake@xxxxxxxxxx>
[Comments merged properly with recent commit "error: Document Error
API usage rules", and edited for clarity.  Put ERRP_AUTO_PROPAGATE()
before its helpers, and touch up style.  Commit message tweaked.]
Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
---
  include/qapi/error.h | 160 ++++++++++++++++++++++++++++++++++++++-----
  1 file changed, 141 insertions(+), 19 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3fed49747d..c865a7d2f1 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h

@@ -128,18 +122,26 @@
   *         handle the error...
   *     }
   * when it doesn't, say a void function:
+ *     ERRP_AUTO_PROPAGATE();
+ *     foo(arg, errp);
+ *     if (*errp) {
+ *         handle the error...
+ *     }
+ * More on ERRP_AUTO_PROPAGATE() below.
+ *
+ * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:

exists

   *     Error *err = NULL;
   *     foo(arg, &err);
   *     if (err) {
   *         handle the error...
- *         error_propagate(errp, err);
+ *         error_propagate(errp, err); // deprecated
   *     }
- * Do *not* "optimize" this to
+ * Avoid in new code.  Do *not* "optimize" it to
   *     foo(arg, errp);
   *     if (*errp) { // WRONG!
   *         handle the error...
   *     }
- * because errp may be NULL!
+ * because errp may be NULL!  Guard with ERRP_AUTO_PROPAGATE().

maybe:

because errp may be NULL without the ERRP_AUTO_PROPAGATE() guard.

   *
   * But when all you do with the error is pass it on, please use
   *     foo(arg, errp);
@@ -158,6 +160,19 @@
   *         handle the error...
   *     }
   *
+ * Pass an existing error to the caller:

+ * = Converting to ERRP_AUTO_PROPAGATE() =
+ *
+ * To convert a function to use ERRP_AUTO_PROPAGATE():
+ *
+ * 0. If the Error ** parameter is not named @errp, rename it to
+ *    @errp.
+ *
+ * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
+ *    the beginning of the function.  This makes @errp safe to use.
+ *
+ * 2. Replace &err by errp, and err by *errp.  Delete local variable
+ *    @err.
+ *
+ * 3. Delete error_propagate(errp, *errp), replace
+ *    error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
+ *

Why a comma here?

+ * 4. Ensure @errp is valid at return: when you destroy *errp, set
+ *    errp = NULL.
+ *
+ * Example:
+ *
+ *     bool fn(..., Error **errp)
+ *     {
+ *         Error *err = NULL;
+ *
+ *         foo(arg, &err);
+ *         if (err) {
+ *             handle the error...
+ *             error_propagate(errp, err);
+ *             return false;
+ *         }
+ *         ...
+ *     }
+ *
+ * becomes
+ *
+ *     bool fn(..., Error **errp)
+ *     {
+ *         ERRP_AUTO_PROPAGATE();
+ *
+ *         foo(arg, errp);
+ *         if (*errp) {
+ *             handle the error...
+ *             return false;
+ *         }
+ *         ...
+ *     }

Do we want the example to show the use of error_free and *errp = NULL?

Otherwise, this is looking good to me. It will need a tweak if we go with the shorter name ERRP_GUARD, but I like that idea.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.