|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
> 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.
That's why we review patches :)
>> 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.
Nevermind. We'd have to add do ... while semicolon armor, and then it's
hardly clearer anymore.
> Still, no objections for s/errp == NULL/!errp/ and we need s/*errp ==
> error_fatal/errp == &error_fatal/ for sure.
Okay!
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |