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