On Tue, 08 Oct 2019 18:03:13 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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.
>
FWIW I've already posted this:
Author: Greg Kurz <gr...@kaod.org>
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 <gr...@kaod.org>
Message-id: <156871563702.196432.5964411202152101367.st...@bahia.lan>
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.
