Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>]

21.02.2020 12:19, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>
> > Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> > functions with an errp OUT parameter.
> >
> > It has three goals:
> >
> > 1. Fix issue with error_fatal and 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 and error_propagate: when we wrap
> > error_abort by local_err+error_propagate, the 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 us to [3.] drop
> > the 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, later patches will add invocations
> > of this macro at the start of functions with either use
> > error_prepend/error_append_hint (solving 1) or which use
> > local_err+error_propagate to check errors, switching those
> > functions to use *errp instead (solving 2 and 3).
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
> > Reviewed-by: Greg Kurz <groug@xxxxxxxx>
> > Reviewed-by: Eric Blake <eblake@xxxxxxxxxx>
> > ---
> >
> > CC: Eric Blake <eblake@xxxxxxxxxx>
> > CC: Kevin Wolf <kwolf@xxxxxxxxxx>
> > CC: Max Reitz <mreitz@xxxxxxxxxx>
> > CC: Greg Kurz <groug@xxxxxxxx>
> > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
> > CC: Paul Durrant <paul@xxxxxxx>
> > CC: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> > CC: "Philippe Mathieu-Daudé" <philmd@xxxxxxxxxx>
> > CC: Laszlo Ersek <lersek@xxxxxxxxxx>
> > CC: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > CC: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > CC: Markus Armbruster <armbru@xxxxxxxxxx>
> > CC: Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>
> > CC: qemu-block@xxxxxxxxxx
> > CC: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >
> >   include/qapi/error.h | 83 +++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > index d34987148d..b9452d4806 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, the use ERRP_AUTO_PROPAGATE 
> > macro
> > + * instead (defined below).
> > + * It's deprecated because of two things:
> > + *
> > + * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
> > + * local_err+error_propagate, the 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);
> Let's explain what should be done *first*, and only then talk about the
> deprecated pattern and how to convert it to current usage.
>
> > @@ -348,6 +392,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 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.
> Tradeoff: we gain more useful backtraces, we lose message improvements
> from error_prepend(), error_append_hint() and such, if any.  Makes
> sense.
>
> > + */
> The comment's contents looks okay to me.  I'll want to tweak formatting
> to better blend in with the rest of this file, but let's not worry about
> that now.
>
> > +#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.
> *errp == error_fatal tests *errp == NULL, which is not what you want.
> You need to test errp == &error_fatal, just like error_handle_fatal().
Oops, great bug) And nobody noticed before) Of course, you are right.

> Superfluous parenthesis around the first operand of ?:.
>
> Wouldn't
>
>     #define ERRP_AUTO_PROPAGATE()                                  \
>         g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>         if (!errp || errp == &error_fatal) {                       \
>             errp = &_auto_errp_prop.local_err;                     \
>         }
>
> be clearer?
Hmm, notation with "if" will allow omitting ';' after macro invocation, which 
seems not good..
And if I'm not wrong we've already discussed it somewhere in previous versions.

Still, no objections for s/errp == NULL/!errp/ and we need s/*errp == 
error_fatal/errp == &error_fatal/ for sure.

--
Best regards,
Vladimir

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel