[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 02/11] error: auto propagated local_err
From: |
Greg Kurz |
Subject: |
Re: [PATCH v6 02/11] error: auto propagated local_err |
Date: |
Wed, 15 Jan 2020 19:33:12 +0100 |
On Fri, 10 Jan 2020 22:41:49 +0300
Vladimir Sementsov-Ogievskiy <address@hidden> wrote:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: 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 & 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 doesn'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 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.).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>
LGTM
Reviewed-by: Greg Kurz <address@hidden>
> CC: Cornelia Huck <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Max Reitz <address@hidden>
> CC: Greg Kurz <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Stefano Stabellini <address@hidden>
> CC: Anthony Perard <address@hidden>
> CC: Paul Durrant <address@hidden>
> CC: "Philippe Mathieu-Daudé" <address@hidden>
> CC: Laszlo Ersek <address@hidden>
> CC: Gerd Hoffmann <address@hidden>
> CC: Stefan Berger <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Michael Roth <address@hidden>
> CC: address@hidden
> CC: address@hidden
>
> include/qapi/error.h | 84 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index fa8d51fd6d..532b9afb9e 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -78,7 +78,7 @@
> * Call a function treating errors as fatal:
> * foo(arg, &error_fatal);
> *
> - * 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
> + * 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
> + * and not to the place where error happened.
> + *
> + * 2. A lot of extra code of the same pattern
> + *
> + * How to update old code to use ERRP_AUTO_PROPAGATE?
> + *
> + * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
> + * than you may safely dereference errp to check errors and do not need any
> + * additional local Error variables or calls to error_propagate().
> + *
> + * Example:
> + *
> + * old code
> + *
> + * void fn(..., Error **errp) {
> + * Error *err = NULL;
> + * foo(arg, &err);
> + * if (err) {
> + * handle the error...
> + * error_propagate(errp, err);
> + * return;
> + * }
> + * ...
> + * }
> + *
> + * updated code
> + *
> + * void fn(..., Error **errp) {
> + * ERRP_AUTO_PROPAGATE();
> + * foo(arg, errp);
> + * if (*errp) {
> + * handle the error...
> + * return;
> + * }
> + * ...
> + * }
> + *
> + *
> * Receive and accumulate multiple errors (first one wins):
> * Error *err = NULL, *local_err = NULL;
> * foo(arg, &err);
> @@ -348,6 +392,44 @@ 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_AUTO_PROPAGATE
> + *
> + * 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.
> + *
> + * 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).
> + *
> + * 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.
- [PATCH v6 00/11] error: auto propagated local_err part I, Vladimir Sementsov-Ogievskiy, 2020/01/10
- [PATCH v6 06/11] pflash: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2020/01/10
- [PATCH v6 10/11] nbd: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2020/01/10
- [PATCH v6 01/11] qapi/error: add (Error **errp) cleaning APIs, Vladimir Sementsov-Ogievskiy, 2020/01/10
- [PATCH v6 02/11] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2020/01/10
- [PATCH v6 03/11] scripts: add coccinelle script to use auto propagated errp, Vladimir Sementsov-Ogievskiy, 2020/01/10
- [PATCH v6 11/11] xen: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2020/01/10
- Re: [Xen-devel] [PATCH v6 00/11] error: auto propagated local_err part I, no-reply, 2020/01/10
- Re: [PATCH v6 00/11] error: auto propagated local_err part I, Vladimir Sementsov-Ogievskiy, 2020/01/30