diff mbox series

[v7,02/11] error: auto propagated local_err

Message ID 20200131130118.1716-3-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series error: auto propagated local_err part I | expand

Commit Message

Vladimir Sementsov-Ogievskiy Jan. 31, 2020, 1:01 p.m. UTC
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(-)

Comments

Markus Armbruster Feb. 21, 2020, 9:19 a.m. UTC | #1
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().

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?
Vladimir Sementsov-Ogievskiy Feb. 21, 2020, 9:42 a.m. UTC | #2
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.
Eric Blake Feb. 21, 2020, 2:29 p.m. UTC | #3
On 2/21/20 3:42 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +#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.

Sorry I missed it in my earlier looks.

> 
>>
>> 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..

Then wrap it:

g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
do { \
   if (!errp || errp == &error_fata) {
     errp = &_auto_errp_prop.local_err; \
   } \
while (0)


> And if I'm not wrong we've already discussed it somewhere in previous 
> versions.

The original use of ?: stems from my suggestion on an earlier revision 
when we were still trying to pack everything into two consecutive 
declaration lines, rather than a declaration and a statement (as ?: is 
necessary for conditionals in declarations).  But since then, we decided 
to go with a statement (we require a C99 compiler, so declaration after 
statement is supported by our compiler, even if our coding style 
currently avoids it where possible), so as long as we support 
statements, we might as well go with a legible statement instead of 
insisting on the compact ?: form.
Markus Armbruster Feb. 21, 2020, 4:23 p.m. UTC | #4
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 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.

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!
diff mbox series

Patch

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);
@@ -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.
+ */
+#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.