[RFC,v2,1/9] error: auto propagated local_err

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 23, 2019, 4:12 p.m. UTC

Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
any function with errp parameter.

It has three goals:

1. Fix issue with error_fatal & error_append_hint: user can't see these
hints, because exit() happens in error_setg earlier than hint is
appended. [Reported by Greg Kurz]

2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself don't fix the issue, but it allows to [3.] drop all
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 should be merely updated to
return int error code).

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Eric Blake Sept. 23, 2019, 7:58 p.m. UTC | #1

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> any function with errp parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself don't fix the issue, but it allows to [3.] drop all

doesn't

> 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 should be merely updated to

Maybe:

s/should/could/

> return int error code).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>  include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..f6f4fa0fac 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
>                          ErrorClass err_class, const char *fmt, ...)
>      GCC_FMT_ATTR(6, 7);
>  
> +typedef struct ErrorPropagator {
> +    Error *local_err;
> +    Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +    error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_FUNCTION_BEGIN
> + *
> + * This macro MUST be the first line of EACH function with Error **errp
> + * parameter.

Maybe s/EACH function/ANY non-empty function/ (allowing our stub
functions to be exemptions).

> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to

s/to/to a/

> + * local Error object, which will be automatically propagated to original

s/to/to the/

> + * 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 append hint (by error_append_hint)

s/append hint/append hints/

> + * (as, if it was error_fatal, we swapped it by local_error to be

s/by local_error/with a local error/

> + * propagated on cleanup).
> + *
> + * Note: we don't wrap error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.

s/wrap/wrap the/

> + */
> +#define ERRP_FUNCTION_BEGIN() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +Error **__local_errp_unused __attribute__ ((unused)) = \
> +    (errp = (errp == NULL || *errp == error_fatal ? \
> +             &__auto_errp_prop.local_err : errp))

I'm not sold on why we need the dummy declaration (yeah, I know it's so
that you don't have to fight the battle of mixing declarations and
statements - but this is in a macro call whose usage LOOKS like a
statement rather than a declaration, so we're already on fuzzy ground).
We could make this macro expansion one line shorter and still be
correct, but I'm not going to insist (we'll see what consensus is,
and/or what Markus says).

> +
> +
>  /*
>   * Special error destination to abort on error.
>   * See error_setg() and error_propagate() for details.
>

Patch

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01..f6f4fa0fac 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -322,6 +322,43 @@  void error_set_internal(Error **errp,
                         ErrorClass err_class, const char *fmt, ...)
     GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+    Error *local_err;
+    Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+    error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_FUNCTION_BEGIN
+ *
+ * This macro MUST be the first line of EACH function with Error **errp
+ * parameter.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to
+ * local Error object, which will be automatically propagated to 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 append hint (by error_append_hint)
+ * (as, if it was error_fatal, we swapped it by local_error to be
+ * propagated on cleanup).
+ *
+ * Note: we don't wrap error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_FUNCTION_BEGIN() \
+g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
+Error **__local_errp_unused __attribute__ ((unused)) = \
+    (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.