Re: [PATCH v4 04/31] error: auto propagated local_err

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 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).

Starting with stating your goals is an excellent idea.  But I'd love to
next read a high-level description of how your patch achieves or enables
achieving these goals.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
[...]
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 9376f59c35..02f967ac1d 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_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. 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 append hints (by error_append_hint)
> + * (as, if it was error_fatal, we swapped it with a local_error to be
> + * propagated on cleanup).

Well, appending hints was always safe, it just didn't work with
&error_fatal.  Don't worry about that now, I'll probably want to polish
this contract comment a bit anyway, but later.

> + *
> + * 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}; \

Took me a second to realize: the macro works, because the initializer
implicitly initializes .local_error = NULL.

__auto_errp_prop is an identifier reserved for any use.  I think we
could use _auto_errp_prop, which is only reserved for use as identifiers
with file scope in both the ordinary and tag name spaces.  See ISO/IEC
9899:1999 7.1.3 Reserved identifiers.

> +errp = ((errp == NULL || *errp == error_fatal) ? \
> +    &__auto_errp_prop.local_err : errp)
> +

Please indent multi-line macros like elsewhere in this file:

#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.

To be honest, the cover letter left me a bit skeptical, but now I think
you might be up to something.  Let's see how the patches putting the
macro to use come out.