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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

21.02.2020 12:19, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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@virtuozzo.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Greg Kurz <groug@kaod.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Anthony Perard <anthony.perard@citrix.com>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Stefan Berger <stefanb@linux.ibm.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> CC: qemu-block@nongnu.org
>> CC: xen-devel@lists.xenproject.org
>>
>>   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