[RFC,v5,024/126] error: auto propagated local_err

Commit Message

Vladimir Sementsov-Ogievskiy Oct. 11, 2019, 4:04 p.m. UTC

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

To achieve these goals, we need to add invocation of the macro at start
of functions, which needs error_prepend/error_append_hint (1.); add
invocation of the macro at start of functions which do
local_err+error_propagate scenario the check errors, drop local errors
from them and just use *errp instead (2., 3.).

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
Reviewed-by: Eric Blake <[e-mail]>
---

CC: Gerd Hoffmann <[e-mail]>
CC: "Gonglei (Arei)" <[e-mail]>
CC: Eduardo Habkost <[e-mail]>
CC: Igor Mammedov <[e-mail]>
CC: Laurent Vivier <[e-mail]>
CC: Amit Shah <[e-mail]>
CC: Kevin Wolf <[e-mail]>
CC: Max Reitz <[e-mail]>
CC: John Snow <[e-mail]>
CC: Ari Sundholm <[e-mail]>
CC: Pavel Dovgalyuk <[e-mail]>
CC: Paolo Bonzini <[e-mail]>
CC: Stefan Hajnoczi <[e-mail]>
CC: Fam Zheng <[e-mail]>
CC: Stefan Weil <[e-mail]>
CC: Ronnie Sahlberg <[e-mail]>
CC: Peter Lieven <[e-mail]>
CC: Eric Blake <[e-mail]>
CC: "Denis V. Lunev" <[e-mail]>
CC: Markus Armbruster <[e-mail]>
CC: Alberto Garcia <[e-mail]>
CC: Jason Dillaman <[e-mail]>
CC: Wen Congyang <[e-mail]>
CC: Xie Changlong <[e-mail]>
CC: Liu Yuan <[e-mail]>
CC: "Richard W.M. Jones" <[e-mail]>
CC: Jeff Cody <[e-mail]>
CC: "Marc-André Lureau" <[e-mail]>
CC: "Daniel P. Berrangé" <[e-mail]>
CC: Richard Henderson <[e-mail]>
CC: Greg Kurz <[e-mail]>
CC: "Michael S. Tsirkin" <[e-mail]>
CC: Marcel Apfelbaum <[e-mail]>
CC: Beniamino Galvani <[e-mail]>
CC: Peter Maydell <[e-mail]>
CC: "Cédric Le Goater" <[e-mail]>
CC: Andrew Jeffery <[e-mail]>
CC: Joel Stanley <[e-mail]>
CC: Andrew Baumann <[e-mail]>
CC: "Philippe Mathieu-Daudé" <[e-mail]>
CC: Antony Pavlov <[e-mail]>
CC: Jean-Christophe Dubois <[e-mail]>
CC: Peter Chubb <[e-mail]>
CC: Subbaraya Sundeep <[e-mail]>
CC: Eric Auger <[e-mail]>
CC: Alistair Francis <[e-mail]>
CC: "Edgar E. Iglesias" <[e-mail]>
CC: Stefano Stabellini <[e-mail]>
CC: Anthony Perard <[e-mail]>
CC: Paul Durrant <[e-mail]>
CC: Paul Burton <[e-mail]>
CC: Aleksandar Rikalo <[e-mail]>
CC: Chris Wulff <[e-mail]>
CC: Marek Vasut <[e-mail]>
CC: David Gibson <[e-mail]>
CC: Cornelia Huck <[e-mail]>
CC: Halil Pasic <[e-mail]>
CC: Christian Borntraeger <[e-mail]>
CC: "Hervé Poussineau" <[e-mail]>
CC: Xiao Guangrong <[e-mail]>
CC: Aurelien Jarno <[e-mail]>
CC: Aleksandar Markovic <[e-mail]>
CC: Mark Cave-Ayland <[e-mail]>
CC: Jason Wang <[e-mail]>
CC: Laszlo Ersek <[e-mail]>
CC: Yuval Shaia <[e-mail]>
CC: Palmer Dabbelt <[e-mail]>
CC: Sagar Karandikar <[e-mail]>
CC: Bastian Koppelmann <[e-mail]>
CC: David Hildenbrand <[e-mail]>
CC: Thomas Huth <[e-mail]>
CC: Eric Farman <[e-mail]>
CC: Matthew Rosato <[e-mail]>
CC: Hannes Reinecke <[e-mail]>
CC: Michael Walle <[e-mail]>
CC: Artyom Tarasenko <[e-mail]>
CC: Stefan Berger <[e-mail]>
CC: Samuel Thibault <[e-mail]>
CC: Alex Williamson <[e-mail]>
CC: Tony Krowiak <[e-mail]>
CC: Pierre Morel <[e-mail]>
CC: Michael Roth <[e-mail]>
CC: Hailiang Zhang <[e-mail]>
CC: Juan Quintela <[e-mail]>
CC: "Dr. David Alan Gilbert" <[e-mail]>
CC: Luigi Rizzo <[e-mail]>
CC: Giuseppe Lettieri <[e-mail]>
CC: Vincenzo Maffione <[e-mail]>
CC: Jan Kiszka <[e-mail]>
CC: Anthony Green <[e-mail]>
CC: Stafford Horne <[e-mail]>
CC: Guan Xuetao <[e-mail]>
CC: Max Filippov <[e-mail]>
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]

 include/qapi/error.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Marc-André Lureau Nov. 8, 2019, 9:10 p.m. UTC | #1

On Fri, Oct 11, 2019 at 10:11 PM Vladimir Sementsov-Ogievskiy
<[e-mail]> wrote:
>
> 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).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> Reviewed-by: Eric Blake <[e-mail]>
> ---
>
> CC: Gerd Hoffmann <[e-mail]>
> CC: "Gonglei (Arei)" <[e-mail]>
> CC: Eduardo Habkost <[e-mail]>
> CC: Igor Mammedov <[e-mail]>
> CC: Laurent Vivier <[e-mail]>
> CC: Amit Shah <[e-mail]>
> CC: Kevin Wolf <[e-mail]>
> CC: Max Reitz <[e-mail]>
> CC: John Snow <[e-mail]>
> CC: Ari Sundholm <[e-mail]>
> CC: Pavel Dovgalyuk <[e-mail]>
> CC: Paolo Bonzini <[e-mail]>
> CC: Stefan Hajnoczi <[e-mail]>
> CC: Fam Zheng <[e-mail]>
> CC: Stefan Weil <[e-mail]>
> CC: Ronnie Sahlberg <[e-mail]>
> CC: Peter Lieven <[e-mail]>
> CC: Eric Blake <[e-mail]>
> CC: "Denis V. Lunev" <[e-mail]>
> CC: Markus Armbruster <[e-mail]>
> CC: Alberto Garcia <[e-mail]>
> CC: Jason Dillaman <[e-mail]>
> CC: Wen Congyang <[e-mail]>
> CC: Xie Changlong <[e-mail]>
> CC: Liu Yuan <[e-mail]>
> CC: "Richard W.M. Jones" <[e-mail]>
> CC: Jeff Cody <[e-mail]>
> CC: "Marc-André Lureau" <[e-mail]>
> CC: "Daniel P. Berrangé" <[e-mail]>
> CC: Richard Henderson <[e-mail]>
> CC: Greg Kurz <[e-mail]>
> CC: "Michael S. Tsirkin" <[e-mail]>
> CC: Marcel Apfelbaum <[e-mail]>
> CC: Beniamino Galvani <[e-mail]>
> CC: Peter Maydell <[e-mail]>
> CC: "Cédric Le Goater" <[e-mail]>
> CC: Andrew Jeffery <[e-mail]>
> CC: Joel Stanley <[e-mail]>
> CC: Andrew Baumann <[e-mail]>
> CC: "Philippe Mathieu-Daudé" <[e-mail]>
> CC: Antony Pavlov <[e-mail]>
> CC: Jean-Christophe Dubois <[e-mail]>
> CC: Peter Chubb <[e-mail]>
> CC: Subbaraya Sundeep <[e-mail]>
> CC: Eric Auger <[e-mail]>
> CC: Alistair Francis <[e-mail]>
> CC: "Edgar E. Iglesias" <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Anthony Perard <[e-mail]>
> CC: Paul Durrant <[e-mail]>
> CC: Paul Burton <[e-mail]>
> CC: Aleksandar Rikalo <[e-mail]>
> CC: Chris Wulff <[e-mail]>
> CC: Marek Vasut <[e-mail]>
> CC: David Gibson <[e-mail]>
> CC: Cornelia Huck <[e-mail]>
> CC: Halil Pasic <[e-mail]>
> CC: Christian Borntraeger <[e-mail]>
> CC: "Hervé Poussineau" <[e-mail]>
> CC: Xiao Guangrong <[e-mail]>
> CC: Aurelien Jarno <[e-mail]>
> CC: Aleksandar Markovic <[e-mail]>
> CC: Mark Cave-Ayland <[e-mail]>
> CC: Jason Wang <[e-mail]>
> CC: Laszlo Ersek <[e-mail]>
> CC: Yuval Shaia <[e-mail]>
> CC: Palmer Dabbelt <[e-mail]>
> CC: Sagar Karandikar <[e-mail]>
> CC: Bastian Koppelmann <[e-mail]>
> CC: David Hildenbrand <[e-mail]>
> CC: Thomas Huth <[e-mail]>
> CC: Eric Farman <[e-mail]>
> CC: Matthew Rosato <[e-mail]>
> CC: Hannes Reinecke <[e-mail]>
> CC: Michael Walle <[e-mail]>
> CC: Artyom Tarasenko <[e-mail]>
> CC: Stefan Berger <[e-mail]>
> CC: Samuel Thibault <[e-mail]>
> CC: Alex Williamson <[e-mail]>
> CC: Tony Krowiak <[e-mail]>
> CC: Pierre Morel <[e-mail]>
> CC: Michael Roth <[e-mail]>
> CC: Hailiang Zhang <[e-mail]>
> CC: Juan Quintela <[e-mail]>
> CC: "Dr. David Alan Gilbert" <[e-mail]>
> CC: Luigi Rizzo <[e-mail]>
> CC: Giuseppe Lettieri <[e-mail]>
> CC: Vincenzo Maffione <[e-mail]>
> CC: Jan Kiszka <[e-mail]>
> CC: Anthony Green <[e-mail]>
> CC: Stafford Horne <[e-mail]>
> CC: Guan Xuetao <[e-mail]>
> CC: Max Filippov <[e-mail]>
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
>
>  include/qapi/error.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d6898d833b..47238d9065 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -345,6 +345,44 @@ 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 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).

Nice improvements. Minor drawback, the abort()/exit() will now take
place when going out of scope and running the cleanup instead of error
location. Not a big problem I guess.

> + *
> + * 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.
> --
> 2.21.0
>
>

Reviewed-by: Marc-André Lureau <[e-mail]>

Eric Blake Nov. 8, 2019, 10:45 p.m. UTC | #2

On 11/8/19 3:10 PM, Marc-André Lureau wrote:

>> +/*
>> + * 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 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).
> 
> Nice improvements. Minor drawback, the abort()/exit() will now take
> place when going out of scope and running the cleanup instead of error
> location. Not a big problem I guess.

Your assessment is not quite right:

Any abort() will happen at the leaf node (because we are no longer 
wrapping thing into a local err and skipping error_propagate altogether 
for &error_abort).

You are correct that any exit() will now happen during cleanup, but that 
is an undetectable change (there is no stack trace present for 
&error_fatal, so calling error_propagate at a later point in time does 
not affect the observable end behavior).

Markus Armbruster Dec. 4, 2019, 2:59 p.m. UTC | #3

Vladimir Sementsov-Ogievskiy <[e-mail]> 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.

I get what you mean, but I have plenty of context.

> (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)

The parenthesis is not part of the goal.

> [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, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).

The paragraph talks about two cases: 1. and 2.+3.  Makes me think we
want two paragraphs, each illustrated with an example.

What about you provide the examples, and then I try to polish the prose?

> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> Reviewed-by: Eric Blake <[e-mail]>
> ---
>
> CC: Gerd Hoffmann <[e-mail]>
> CC: "Gonglei (Arei)" <[e-mail]>
> CC: Eduardo Habkost <[e-mail]>
> CC: Igor Mammedov <[e-mail]>
> CC: Laurent Vivier <[e-mail]>
> CC: Amit Shah <[e-mail]>
> CC: Kevin Wolf <[e-mail]>
> CC: Max Reitz <[e-mail]>
> CC: John Snow <[e-mail]>
> CC: Ari Sundholm <[e-mail]>
> CC: Pavel Dovgalyuk <[e-mail]>
> CC: Paolo Bonzini <[e-mail]>
> CC: Stefan Hajnoczi <[e-mail]>
> CC: Fam Zheng <[e-mail]>
> CC: Stefan Weil <[e-mail]>
> CC: Ronnie Sahlberg <[e-mail]>
> CC: Peter Lieven <[e-mail]>
> CC: Eric Blake <[e-mail]>
> CC: "Denis V. Lunev" <[e-mail]>
> CC: Markus Armbruster <[e-mail]>
> CC: Alberto Garcia <[e-mail]>
> CC: Jason Dillaman <[e-mail]>
> CC: Wen Congyang <[e-mail]>
> CC: Xie Changlong <[e-mail]>
> CC: Liu Yuan <[e-mail]>
> CC: "Richard W.M. Jones" <[e-mail]>
> CC: Jeff Cody <[e-mail]>
> CC: "Marc-André Lureau" <[e-mail]>
> CC: "Daniel P. Berrangé" <[e-mail]>
> CC: Richard Henderson <[e-mail]>
> CC: Greg Kurz <[e-mail]>
> CC: "Michael S. Tsirkin" <[e-mail]>
> CC: Marcel Apfelbaum <[e-mail]>
> CC: Beniamino Galvani <[e-mail]>
> CC: Peter Maydell <[e-mail]>
> CC: "Cédric Le Goater" <[e-mail]>
> CC: Andrew Jeffery <[e-mail]>
> CC: Joel Stanley <[e-mail]>
> CC: Andrew Baumann <[e-mail]>
> CC: "Philippe Mathieu-Daudé" <[e-mail]>
> CC: Antony Pavlov <[e-mail]>
> CC: Jean-Christophe Dubois <[e-mail]>
> CC: Peter Chubb <[e-mail]>
> CC: Subbaraya Sundeep <[e-mail]>
> CC: Eric Auger <[e-mail]>
> CC: Alistair Francis <[e-mail]>
> CC: "Edgar E. Iglesias" <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Anthony Perard <[e-mail]>
> CC: Paul Durrant <[e-mail]>
> CC: Paul Burton <[e-mail]>
> CC: Aleksandar Rikalo <[e-mail]>
> CC: Chris Wulff <[e-mail]>
> CC: Marek Vasut <[e-mail]>
> CC: David Gibson <[e-mail]>
> CC: Cornelia Huck <[e-mail]>
> CC: Halil Pasic <[e-mail]>
> CC: Christian Borntraeger <[e-mail]>
> CC: "Hervé Poussineau" <[e-mail]>
> CC: Xiao Guangrong <[e-mail]>
> CC: Aurelien Jarno <[e-mail]>
> CC: Aleksandar Markovic <[e-mail]>
> CC: Mark Cave-Ayland <[e-mail]>
> CC: Jason Wang <[e-mail]>
> CC: Laszlo Ersek <[e-mail]>
> CC: Yuval Shaia <[e-mail]>
> CC: Palmer Dabbelt <[e-mail]>
> CC: Sagar Karandikar <[e-mail]>
> CC: Bastian Koppelmann <[e-mail]>
> CC: David Hildenbrand <[e-mail]>
> CC: Thomas Huth <[e-mail]>
> CC: Eric Farman <[e-mail]>
> CC: Matthew Rosato <[e-mail]>
> CC: Hannes Reinecke <[e-mail]>
> CC: Michael Walle <[e-mail]>
> CC: Artyom Tarasenko <[e-mail]>
> CC: Stefan Berger <[e-mail]>
> CC: Samuel Thibault <[e-mail]>
> CC: Alex Williamson <[e-mail]>
> CC: Tony Krowiak <[e-mail]>
> CC: Pierre Morel <[e-mail]>
> CC: Michael Roth <[e-mail]>
> CC: Hailiang Zhang <[e-mail]>
> CC: Juan Quintela <[e-mail]>
> CC: "Dr. David Alan Gilbert" <[e-mail]>
> CC: Luigi Rizzo <[e-mail]>
> CC: Giuseppe Lettieri <[e-mail]>
> CC: Vincenzo Maffione <[e-mail]>
> CC: Jan Kiszka <[e-mail]>
> CC: Anthony Green <[e-mail]>
> CC: Stafford Horne <[e-mail]>
> CC: Guan Xuetao <[e-mail]>
> CC: Max Filippov <[e-mail]>
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
> CC: [e-mail]
>
>  include/qapi/error.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d6898d833b..47238d9065 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -345,6 +345,44 @@ 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 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.

Missing: update of the big comment starting with "Error reporting system
loosely patterned after Glib's GError."

Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 9:38 a.m. UTC | #4

04.12.2019 17:59, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <[e-mail]> 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.
> 
> I get what you mean, but I have plenty of context.
> 
>> (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)
> 
> The parenthesis is not part of the goal.
> 
>> [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, we need to add invocation of the macro at start
>> of functions, which needs error_prepend/error_append_hint (1.); add
>> invocation of the macro at start of functions which do
>> local_err+error_propagate scenario the check errors, drop local errors
>> from them and just use *errp instead (2., 3.).
> 
> The paragraph talks about two cases: 1. and 2.+3. 

Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
fix achieve 2 and 3 by one action.

> Makes me think we
> want two paragraphs, each illustrated with an example.
> 
> What about you provide the examples, and then I try to polish the prose?

1: error_fatal problem

Assume the following code flow:

int f1(errp) {
    ...
    ret = f2(errp);
    if (ret < 0) {
       error_append_hint(errp, "very useful hint");
       return ret;
    }
    ...
}

Now, if we call f1 with &error_fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.

So, in this case we should use local_err.

2: error_abort problem

Now, consider functions without return value. We normally use local_err
variable to catch failures:

void f1(errp) {
    Error *local_err = NULL;
    ...
    f2(&local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }
    ...
}

Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.

So, we should never wrap error_abort by local_err.

===

Our solution:

- Fixes [1.], adding invocation of new macro into functions with error_appen_hint/error_prepend,
   New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which never
   wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra propagations
   (in fact, error_propagate is called, but returns immediately on first if (!local_err))

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>> Reviewed-by: Eric Blake <[e-mail]>
>> ---
>>
>> CC: Gerd Hoffmann <[e-mail]>
>> CC: "Gonglei (Arei)" <[e-mail]>
>> CC: Eduardo Habkost <[e-mail]>
>> CC: Igor Mammedov <[e-mail]>
>> CC: Laurent Vivier <[e-mail]>
>> CC: Amit Shah <[e-mail]>
>> CC: Kevin Wolf <[e-mail]>
>> CC: Max Reitz <[e-mail]>
>> CC: John Snow <[e-mail]>
>> CC: Ari Sundholm <[e-mail]>
>> CC: Pavel Dovgalyuk <[e-mail]>
>> CC: Paolo Bonzini <[e-mail]>
>> CC: Stefan Hajnoczi <[e-mail]>
>> CC: Fam Zheng <[e-mail]>
>> CC: Stefan Weil <[e-mail]>
>> CC: Ronnie Sahlberg <[e-mail]>
>> CC: Peter Lieven <[e-mail]>
>> CC: Eric Blake <[e-mail]>
>> CC: "Denis V. Lunev" <[e-mail]>
>> CC: Markus Armbruster <[e-mail]>
>> CC: Alberto Garcia <[e-mail]>
>> CC: Jason Dillaman <[e-mail]>
>> CC: Wen Congyang <[e-mail]>
>> CC: Xie Changlong <[e-mail]>
>> CC: Liu Yuan <[e-mail]>
>> CC: "Richard W.M. Jones" <[e-mail]>
>> CC: Jeff Cody <[e-mail]>
>> CC: "Marc-André Lureau" <[e-mail]>
>> CC: "Daniel P. Berrangé" <[e-mail]>
>> CC: Richard Henderson <[e-mail]>
>> CC: Greg Kurz <[e-mail]>
>> CC: "Michael S. Tsirkin" <[e-mail]>
>> CC: Marcel Apfelbaum <[e-mail]>
>> CC: Beniamino Galvani <[e-mail]>
>> CC: Peter Maydell <[e-mail]>
>> CC: "Cédric Le Goater" <[e-mail]>
>> CC: Andrew Jeffery <[e-mail]>
>> CC: Joel Stanley <[e-mail]>
>> CC: Andrew Baumann <[e-mail]>
>> CC: "Philippe Mathieu-Daudé" <[e-mail]>
>> CC: Antony Pavlov <[e-mail]>
>> CC: Jean-Christophe Dubois <[e-mail]>
>> CC: Peter Chubb <[e-mail]>
>> CC: Subbaraya Sundeep <[e-mail]>
>> CC: Eric Auger <[e-mail]>
>> CC: Alistair Francis <[e-mail]>
>> CC: "Edgar E. Iglesias" <[e-mail]>
>> CC: Stefano Stabellini <[e-mail]>
>> CC: Anthony Perard <[e-mail]>
>> CC: Paul Durrant <[e-mail]>
>> CC: Paul Burton <[e-mail]>
>> CC: Aleksandar Rikalo <[e-mail]>
>> CC: Chris Wulff <[e-mail]>
>> CC: Marek Vasut <[e-mail]>
>> CC: David Gibson <[e-mail]>
>> CC: Cornelia Huck <[e-mail]>
>> CC: Halil Pasic <[e-mail]>
>> CC: Christian Borntraeger <[e-mail]>
>> CC: "Hervé Poussineau" <[e-mail]>
>> CC: Xiao Guangrong <[e-mail]>
>> CC: Aurelien Jarno <[e-mail]>
>> CC: Aleksandar Markovic <[e-mail]>
>> CC: Mark Cave-Ayland <[e-mail]>
>> CC: Jason Wang <[e-mail]>
>> CC: Laszlo Ersek <[e-mail]>
>> CC: Yuval Shaia <[e-mail]>
>> CC: Palmer Dabbelt <[e-mail]>
>> CC: Sagar Karandikar <[e-mail]>
>> CC: Bastian Koppelmann <[e-mail]>
>> CC: David Hildenbrand <[e-mail]>
>> CC: Thomas Huth <[e-mail]>
>> CC: Eric Farman <[e-mail]>
>> CC: Matthew Rosato <[e-mail]>
>> CC: Hannes Reinecke <[e-mail]>
>> CC: Michael Walle <[e-mail]>
>> CC: Artyom Tarasenko <[e-mail]>
>> CC: Stefan Berger <[e-mail]>
>> CC: Samuel Thibault <[e-mail]>
>> CC: Alex Williamson <[e-mail]>
>> CC: Tony Krowiak <[e-mail]>
>> CC: Pierre Morel <[e-mail]>
>> CC: Michael Roth <[e-mail]>
>> CC: Hailiang Zhang <[e-mail]>
>> CC: Juan Quintela <[e-mail]>
>> CC: "Dr. David Alan Gilbert" <[e-mail]>
>> CC: Luigi Rizzo <[e-mail]>
>> CC: Giuseppe Lettieri <[e-mail]>
>> CC: Vincenzo Maffione <[e-mail]>
>> CC: Jan Kiszka <[e-mail]>
>> CC: Anthony Green <[e-mail]>
>> CC: Stafford Horne <[e-mail]>
>> CC: Guan Xuetao <[e-mail]>
>> CC: Max Filippov <[e-mail]>
>> CC: [e-mail]
>> CC: [e-mail]
>> CC: [e-mail]
>> CC: [e-mail]
>> CC: [e-mail]
>> CC: [e-mail]
>> CC: [e-mail]
>> CC: [e-mail]
>>
>>   include/qapi/error.h | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index d6898d833b..47238d9065 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -345,6 +345,44 @@ 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 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.
> 
> Missing: update of the big comment starting with "Error reporting system
> loosely patterned after Glib's GError."
>

Markus Armbruster Dec. 5, 2019, 12:36 p.m. UTC | #5

Vladimir Sementsov-Ogievskiy <[e-mail]> writes:

> 04.12.2019 17:59, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <[e-mail]> 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.
>> 
>> I get what you mean, but I have plenty of context.
>> 
>>> (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)
>> 
>> The parenthesis is not part of the goal.
>> 
>>> [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, we need to add invocation of the macro at start
>>> of functions, which needs error_prepend/error_append_hint (1.); add
>>> invocation of the macro at start of functions which do
>>> local_err+error_propagate scenario the check errors, drop local errors
>>> from them and just use *errp instead (2., 3.).
>> 
>> The paragraph talks about two cases: 1. and 2.+3. 
>
> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
> fix achieve 2 and 3 by one action.
>
>> Makes me think we
>> want two paragraphs, each illustrated with an example.
>> 
>> What about you provide the examples, and then I try to polish the prose?
>
> 1: error_fatal problem
>
> Assume the following code flow:
>
> int f1(errp) {
>     ...
>     ret = f2(errp);
>     if (ret < 0) {
>        error_append_hint(errp, "very useful hint");
>        return ret;
>     }
>     ...
> }
>
> Now, if we call f1 with &error_fatal argument and f2 fails, the program
> will exit immediately inside f2, when setting the errp. User will not
> see the hint.
>
> So, in this case we should use local_err.

How does this example look after the transformation?

> 2: error_abort problem
>
> Now, consider functions without return value. We normally use local_err
> variable to catch failures:
>
> void f1(errp) {
>     Error *local_err = NULL;
>     ...
>     f2(&local_err);
>     if (local_err) {
>         error_propagate(errp, local_err);
>         return;
>     }
>     ...
> }
>
> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
> crash dump will point to error_propagate, not to the failure point in f2,
> which complicates debugging.
>
> So, we should never wrap error_abort by local_err.

Likewise.

>
> ===
>
> Our solution:
>
> - Fixes [1.], adding invocation of new macro into functions with error_appen_hint/error_prepend,
>    New macro will wrap error_fatal.
> - Fixes [2.], by switching from hand-written local_err to smart macro, which never
>    wraps error_abort.
> - Handles [3.], by switching to macro, which is less code
> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra propagations
>    (in fact, error_propagate is called, but returns immediately on first if (!local_err))

Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 2:58 p.m. UTC | #6

05.12.2019 15:36, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <[e-mail]> writes:
> 
>> 04.12.2019 17:59, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <[e-mail]> 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.
>>>
>>> I get what you mean, but I have plenty of context.
>>>
>>>> (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)
>>>
>>> The parenthesis is not part of the goal.
>>>
>>>> [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, we need to add invocation of the macro at start
>>>> of functions, which needs error_prepend/error_append_hint (1.); add
>>>> invocation of the macro at start of functions which do
>>>> local_err+error_propagate scenario the check errors, drop local errors
>>>> from them and just use *errp instead (2., 3.).
>>>
>>> The paragraph talks about two cases: 1. and 2.+3.
>>
>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>> fix achieve 2 and 3 by one action.
>>
>>> Makes me think we
>>> want two paragraphs, each illustrated with an example.
>>>
>>> What about you provide the examples, and then I try to polish the prose?
>>
>> 1: error_fatal problem
>>
>> Assume the following code flow:
>>
>> int f1(errp) {
>>      ...
>>      ret = f2(errp);
>>      if (ret < 0) {
>>         error_append_hint(errp, "very useful hint");
>>         return ret;
>>      }
>>      ...
>> }
>>
>> Now, if we call f1 with &error_fatal argument and f2 fails, the program
>> will exit immediately inside f2, when setting the errp. User will not
>> see the hint.
>>
>> So, in this case we should use local_err.
> 
> How does this example look after the transformation?

Good point.

int f1(errp) {
    ERRP_AUTO_PROPAGATE();
    ...
    ret = f2(errp);
    if (ret < 0) {
       error_append_hint(errp, "very useful hint");
       return ret;
    }
    ...
}

- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.

> 
>> 2: error_abort problem
>>
>> Now, consider functions without return value. We normally use local_err
>> variable to catch failures:
>>
>> void f1(errp) {
>>      Error *local_err = NULL;
>>      ...
>>      f2(&local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>      ...
>> }
>>
>> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
>> crash dump will point to error_propagate, not to the failure point in f2,
>> which complicates debugging.
>>
>> So, we should never wrap error_abort by local_err.
> 
> Likewise.

And here:

void f1(errp) {
     ERRP_AUTO_PROPAGATE();
     ...
     f2(errp);
     if (*errp) {
         return;
     }
     ...

- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
   local error is automatically propagated to original one.

> 
>>
>> ===
>>
>> Our solution:
>>
>> - Fixes [1.], adding invocation of new macro into functions with error_appen_hint/error_prepend,
>>     New macro will wrap error_fatal.
>> - Fixes [2.], by switching from hand-written local_err to smart macro, which never
>>     wraps error_abort.
>> - Handles [3.], by switching to macro, which is less code
>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra propagations
>>     (in fact, error_propagate is called, but returns immediately on first if (!local_err))
>

Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 4:36 p.m. UTC | #7

05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2019 15:36, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <[e-mail]> writes:
>>
>>> 04.12.2019 17:59, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <[e-mail]> 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.
>>>>
>>>> I get what you mean, but I have plenty of context.
>>>>
>>>>> (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)
>>>>
>>>> The parenthesis is not part of the goal.
>>>>
>>>>> [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, we need to add invocation of the macro at start
>>>>> of functions, which needs error_prepend/error_append_hint (1.); add
>>>>> invocation of the macro at start of functions which do
>>>>> local_err+error_propagate scenario the check errors, drop local errors
>>>>> from them and just use *errp instead (2., 3.).
>>>>
>>>> The paragraph talks about two cases: 1. and 2.+3.
>>>
>>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>>> fix achieve 2 and 3 by one action.
>>>
>>>> Makes me think we
>>>> want two paragraphs, each illustrated with an example.
>>>>
>>>> What about you provide the examples, and then I try to polish the prose?
>>>
>>> 1: error_fatal problem
>>>
>>> Assume the following code flow:
>>>
>>> int f1(errp) {
>>>       ...
>>>       ret = f2(errp);
>>>       if (ret < 0) {
>>>          error_append_hint(errp, "very useful hint");
>>>          return ret;
>>>       }
>>>       ...
>>> }
>>>
>>> Now, if we call f1 with &error_fatal argument and f2 fails, the program
>>> will exit immediately inside f2, when setting the errp. User will not
>>> see the hint.
>>>
>>> So, in this case we should use local_err.
>>
>> How does this example look after the transformation?
> 
> Good point.
> 
> int f1(errp) {
>      ERRP_AUTO_PROPAGATE();
>      ...
>      ret = f2(errp);
>      if (ret < 0) {
>         error_append_hint(errp, "very useful hint");
>         return ret;
>      }
>      ...
> }
> 
> - nothing changed, only add macro at start. But now errp is safe, if it was
> error_fatal it is wrapped by local error, and will only call exit on automatic
> propagation on f1 finish.
> 
>>
>>> 2: error_abort problem
>>>
>>> Now, consider functions without return value. We normally use local_err
>>> variable to catch failures:
>>>
>>> void f1(errp) {
>>>       Error *local_err = NULL;
>>>       ...
>>>       f2(&local_err);
>>>       if (local_err) {
>>>           error_propagate(errp, local_err);
>>>           return;
>>>       }
>>>       ...
>>> }
>>>
>>> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
>>> crash dump will point to error_propagate, not to the failure point in f2,
>>> which complicates debugging.
>>>
>>> So, we should never wrap error_abort by local_err.
>>
>> Likewise.
> 
> And here:
> 
> void f1(errp) {
>       ERRP_AUTO_PROPAGATE();
>       ...
>       f2(errp);
>       if (*errp) {
>           return;
>       }
>       ...
> 
> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
>     local error is automatically propagated to original one.

and if it was error_abort, it is not wrapped, so will crash where failed.

> 
>>
>>>
>>> ===
>>>
>>> Our solution:
>>>
>>> - Fixes [1.], adding invocation of new macro into functions with error_appen_hint/error_prepend,
>>>      New macro will wrap error_fatal.
>>> - Fixes [2.], by switching from hand-written local_err to smart macro, which never
>>>      wraps error_abort.
>>> - Handles [3.], by switching to macro, which is less code
>>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra propagations
>>>      (in fact, error_propagate is called, but returns immediately on first if (!local_err))
>>
> 
>

Eric Blake Dec. 5, 2019, 5:32 p.m. UTC | #8

On 12/5/19 8:58 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>> What about you provide the examples, and then I try to polish the prose?
>>>
>>> 1: error_fatal problem
>>>
>>> Assume the following code flow:
>>>
>>> int f1(errp) {
>>>       ...
>>>       ret = f2(errp);
>>>       if (ret < 0) {
>>>          error_append_hint(errp, "very useful hint");
>>>          return ret;
>>>       }
>>>       ...
>>> }
>>>
>>> Now, if we call f1 with &error_fatal argument and f2 fails, the program
>>> will exit immediately inside f2, when setting the errp. User will not
>>> see the hint.
>>>
>>> So, in this case we should use local_err.
>>
>> How does this example look after the transformation?

Without ERRP_AUTO_PROPAGATE(), the transformation is a lot of boilerplate:

int f1(errp) {
     Error *err = NULL;
     ret = f2(&err);
     if (ret < 0) {
         error_append_hint(&err, "very useful hint");
         error_propagate(errp, err);
         return ret;
     }
}

what's worse, that boilerplate to solve problem 1 turns out to be...

> 
> Good point.
> 
> int f1(errp) {
>      ERRP_AUTO_PROPAGATE();
>      ...
>      ret = f2(errp);
>      if (ret < 0) {
>         error_append_hint(errp, "very useful hint");
>         return ret;
>      }
>      ...
> }
> 
> - nothing changed, only add macro at start. But now errp is safe, if it was
> error_fatal it is wrapped by local error, and will only call exit on automatic
> propagation on f1 finish.
> 
>>
>>> 2: error_abort problem
>>>
>>> Now, consider functions without return value. We normally use local_err
>>> variable to catch failures:
>>>
>>> void f1(errp) {
>>>       Error *local_err = NULL;
>>>       ...
>>>       f2(&local_err);
>>>       if (local_err) {
>>>           error_propagate(errp, local_err);
>>>           return;
>>>       }
>>>       ...
>>> }

the very same code as the cause of problem 2.

>>>
>>> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
>>> crash dump will point to error_propagate, not to the failure point in f2,
>>> which complicates debugging.
>>>
>>> So, we should never wrap error_abort by local_err.
>>
>> Likewise.
> 
> And here:
> 
> void f1(errp) {
>       ERRP_AUTO_PROPAGATE();
>       ...
>       f2(errp);
>       if (*errp) {
>           return;
>       }
>       ...
> 
> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
>     local error is automatically propagated to original one.

So, the use of ERRP_AUTO_PROPAGATE() solves BOTH problems 1 and 2 - we 
avoid the boilerplate that trades one problem for another, by 
consolidating ALL of the boilerplate into a single-line macro, such that 
error_propagate() no longer needs to be called anywhere except inside 
the ERRP_AUTO_PROPAGATE macro.

> 
>>
>>>
>>> ===
>>>
>>> Our solution:
>>>
>>> - Fixes [1.], adding invocation of new macro into functions with error_appen_hint/error_prepend,
>>>      New macro will wrap error_fatal.
>>> - Fixes [2.], by switching from hand-written local_err to smart macro, which never
>>>      wraps error_abort.
>>> - Handles [3.], by switching to macro, which is less code
>>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra propagations
>>>      (in fact, error_propagate is called, but returns immediately on first if (!local_err))
>>
> 
>

Markus Armbruster Dec. 6, 2019, 8:13 a.m. UTC | #9

Vladimir Sementsov-Ogievskiy <[e-mail]> writes:

> 05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
>> 05.12.2019 15:36, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <[e-mail]> writes:
>>>
>>>> 04.12.2019 17:59, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <[e-mail]> 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.
>>>>>
>>>>> I get what you mean, but I have plenty of context.
>>>>>
>>>>>> (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)
>>>>>
>>>>> The parenthesis is not part of the goal.
>>>>>
>>>>>> [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, we need to add invocation of the macro at start
>>>>>> of functions, which needs error_prepend/error_append_hint (1.); add
>>>>>> invocation of the macro at start of functions which do
>>>>>> local_err+error_propagate scenario the check errors, drop local errors
>>>>>> from them and just use *errp instead (2., 3.).
>>>>>
>>>>> The paragraph talks about two cases: 1. and 2.+3.
>>>>
>>>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>>>> fix achieve 2 and 3 by one action.
>>>>
>>>>> Makes me think we
>>>>> want two paragraphs, each illustrated with an example.
>>>>>
>>>>> What about you provide the examples, and then I try to polish the prose?
>>>>
>>>> 1: error_fatal problem
>>>>
>>>> Assume the following code flow:
>>>>
>>>> int f1(errp) {
>>>>       ...
>>>>       ret = f2(errp);
>>>>       if (ret < 0) {
>>>>          error_append_hint(errp, "very useful hint");
>>>>          return ret;
>>>>       }
>>>>       ...
>>>> }
>>>>
>>>> Now, if we call f1 with &error_fatal argument and f2 fails, the program
>>>> will exit immediately inside f2, when setting the errp. User will not
>>>> see the hint.
>>>>
>>>> So, in this case we should use local_err.
>>>
>>> How does this example look after the transformation?
>> 
>> Good point.
>> 
>> int f1(errp) {
>>      ERRP_AUTO_PROPAGATE();
>>      ...
>>      ret = f2(errp);
>>      if (ret < 0) {
>>         error_append_hint(errp, "very useful hint");
>>         return ret;
>>      }
>>      ...
>> }
>> 
>> - nothing changed, only add macro at start. But now errp is safe, if it was
>> error_fatal it is wrapped by local error, and will only call exit on automatic
>> propagation on f1 finish.
>> 
>>>
>>>> 2: error_abort problem
>>>>
>>>> Now, consider functions without return value. We normally use local_err
>>>> variable to catch failures:
>>>>
>>>> void f1(errp) {
>>>>       Error *local_err = NULL;
>>>>       ...
>>>>       f2(&local_err);
>>>>       if (local_err) {
>>>>           error_propagate(errp, local_err);
>>>>           return;
>>>>       }
>>>>       ...
>>>> }
>>>>
>>>> Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
>>>> crash dump will point to error_propagate, not to the failure point in f2,
>>>> which complicates debugging.
>>>>
>>>> So, we should never wrap error_abort by local_err.
>>>
>>> Likewise.
>> 
>> And here:
>> 
>> void f1(errp) {
>>       ERRP_AUTO_PROPAGATE();
>>       ...
>>       f2(errp);
>>       if (*errp) {
>>           return;
>>       }
>>       ...
>> 
>> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
>>     local error is automatically propagated to original one.
>
> and if it was error_abort, it is not wrapped, so will crash where failed.

We still need to avoid passing on unwrapped @errp when we want to ignore
some errors, as in fit_load_fdt(), but that should be quite rare.
Doesn't invalidate your approach.

[...]

Patch

diff --git a/include/qapi/error.h b/include/qapi/error.h
index d6898d833b..47238d9065 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -345,6 +345,44 @@  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 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.