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

[Greg Kurz]

On Tue, 08 Oct 2019 18:03:13 +0200
Markus Armbruster <address@hidden> wrote:

> Vladimir Sementsov-Ogievskiy <address@hidden> 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 <address@hidden>
> > ---
> [...]
> > 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.
> 

FWIW I've already posted this:

Author: Greg Kurz <address@hidden>
Date:   Mon Oct 7 15:45:46 2019 +0200

    error: Update error_append_hint()'s documenation
    
    error_setg() and error_propagate(), as well as their variants, cause
    QEMU to terminate when called with &error_fatal or &error_abort. This
    prevents to add hints since error_append_hint() isn't even called in
    this case.
    
    It means that error_append_hint() should only be used with a local
    error object, and then propagate this local error to the caller.
    
    Document this in <qapi/error.h> .
    
    Signed-off-by: Greg Kurz <address@hidden>

Message-id: <address@hidden>
https://patchwork.ozlabs.org/patch/1163278/

> > + *
> > + * 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.