[Top][All Lists]

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

Re: [PATCH v6 02/11] error: auto propagated local_err

From: Eric Blake
Subject: Re: [PATCH v6 02/11] error: auto propagated local_err
Date: Fri, 17 Jan 2020 08:18:35 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1

On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote:
Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.

s/with/with an/

It has three goals:

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

maybe s/&/and/ so it doesn't look like the C & operator.

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 & error_propagate: when we wrap

and again

error_abort by local_err+error_propagate, resulting coredump will

s/,/, the/

refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows to [3.] drop all

s/allows/allows us/

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, we need to add invocation of the macro at start
of functions, which needs error_prepend/error_append_hint (1.); add
invocation of the macro at start of functions which do
local_err+error_propagate scenario the check errors, drop local errors
from them and just use *errp instead (2., 3.).

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 <address@hidden>

- * Receive an error and pass it on to the caller:
+ * Receive an error and pass it on to the caller (DEPRECATED*):
   *     Error *err = NULL;
   *     foo(arg, &err);
   *     if (err) {
@@ -98,6 +98,50 @@
   *     foo(arg, errp);
   * for readability.
+ * DEPRECATED* This pattern is deprecated now, use ERRP_AUTO_PROPAGATE macro

s/use/use the/

+ * instead (defined below).
+ * It's deprecated because of two things:
+ *
+ * 1. Issue with error_abort & error_propagate: when we wrap error_abort by


+ * local_err+error_propagate, resulting coredump will refer to error_propagate

s/,/, the/

+ * and not to the place where error happened.
+ *
+ * 2. A lot of extra code of the same pattern
+ *

+ *
+ * This macro is created to be the first line of a function which use
+ * Error **errp parameter to report error. It's needed only in cases where we
+ * want to use error_prepend, error_append_hint or dereference *errp. It's
+ * still safe (but useless) in other cases.

It doesn't _have_ to be the first line to compile (we require C99+ compilers, which allow declarations after statements); but rather because it makes it easier for our Coccinelle conversion script to catch outliers. But I think this text is okay, without calling out that extra information (maybe the commit message should mention it, though).

+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information (by error_prepend or
+ * error_append_hint)
+ * (as, if it was error_fatal, we swapped it with a local_error to be
+ * propagated on cleanup).

double () () looks odd, as does the mid-sentence newline.

+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE()                                  \
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
+    errp = ((errp == NULL || *errp == error_fatal)             \
+            ? &_auto_errp_prop.local_err : errp)
   * Special error destination to abort on error.
   * See error_setg() and error_propagate() for details.

The macro itself is fine, my comments are solely on the commit message and comments. Depending on how much cleanup Markus is willing to do rather than require a respin, you can add:

Reviewed-by: Eric Blake <address@hidden>

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]