Vladimir Sementsov-Ogievskiy <[email protected]> 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 <[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). > + * > + * 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."
