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

[Markus Armbruster <armbru@redhat.com>]

Greg Kurz <groug@kaod.org> writes:

> On Tue, 08 Oct 2019 18:03:13 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.
>> 
>
> FWIW I've already posted this:
>
> Author: Greg Kurz <groug@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 <groug@kaod.org>
>
> Message-id: <156871563702.196432.5964411202152101367.stgit@bahia.lan>
> https://patchwork.ozlabs.org/patch/1163278/

I marked your series containing this patch for review.  I decided to
review this one first.  I clearly need look at yours as well before I
can advise on how to proceed.

[...]