Message ID | 20200110194158.14190-3-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | error: auto propagated local_err part I | expand |
On Fri, 10 Jan 2020 at 19:42, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: [snip] > +/* > + * 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) \ Perhaps !errp rather than errp == NULL, for brevity. Paul > + ? &_auto_errp_prop.local_err : errp) > + > /* > * Special error destination to abort on error. > * See error_setg() and error_propagate() for details. > -- > 2.21.0 >
13.01.2020 11:50, Paul Durrant wrote: > On Fri, 10 Jan 2020 at 19:42, Vladimir Sementsov-Ogievskiy > <vsementsov@virtuozzo.com> wrote: > [snip] >> +/* >> + * 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) \ > > Perhaps !errp rather than errp == NULL, for brevity. > I mostly prefer !ptr notation.. But may be here, I'd keep it as is, to stress special-casing NULL in this non-trivial place.. And it is in good relation with phrasing "If errp is NULL or points to error_fatal". But !errp is OK for me to. Let it be as Markus prefer, he is maintainer. > >> + ? &_auto_errp_prop.local_err : errp) >> + >> /* >> * Special error destination to abort on error. >> * See error_setg() and error_propagate() for details. >> -- >> 2.21.0 >>
On Fri, 10 Jan 2020 22:41:49 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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 <vsementsov@virtuozzo.com> > --- > LGTM Reviewed-by: Greg Kurz <groug@kaod.org> > CC: Cornelia Huck <cohuck@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: Stefan Hajnoczi <stefanha@redhat.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Anthony Perard <anthony.perard@citrix.com> > CC: Paul Durrant <paul@xen.org> > 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 | 84 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 83 insertions(+), 1 deletion(-) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index fa8d51fd6d..532b9afb9e 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, use ERRP_AUTO_PROPAGATE macro > + * instead (defined below). > + * It's deprecated because of two things: > + * > + * 1. 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. > + * > + * 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,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 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.
On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote: > Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of > functions with errp OUT parameter. s/with/with an/ > > It has three goals: > > 1. Fix issue with error_fatal & error_prepend/error_append_hint: user maybe s/&/and/ so it doesn't look like the C & operator. > 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 and again > error_abort by local_err+error_propagate, resulting coredump will s/,/, the/ > 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 s/allows/allows us/ s/all/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, 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.). 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> > --- > > - * 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, use ERRP_AUTO_PROPAGATE macro s/use/use the/ > + * instead (defined below). > + * It's deprecated because of two things: > + * > + * 1. Issue with error_abort & error_propagate: when we wrap error_abort by s/&/and/ > + * local_err+error_propagate, resulting coredump will refer to error_propagate s/,/, the/ > + * and not to the place where error happened. > + * > + * 2. A lot of extra code of the same pattern > + * > +/* > + * 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. It doesn't _have_ to be the first line to compile (we require C99+ compilers, which allow declarations after statements); but rather because it makes it easier for our Coccinelle conversion script to catch outliers. But I think this text is okay, without calling out that extra information (maybe the commit message should mention it, though). > + * > + * 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). double () () looks odd, as does the mid-sentence newline. > + * > + * 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. > The macro itself is fine, my comments are solely on the commit message and comments. Depending on how much cleanup Markus is willing to do rather than require a respin, you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/include/qapi/error.h b/include/qapi/error.h index fa8d51fd6d..532b9afb9e 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, use ERRP_AUTO_PROPAGATE macro + * instead (defined below). + * It's deprecated because of two things: + * + * 1. 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. + * + * 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,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 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.
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 <vsementsov@virtuozzo.com> --- CC: Cornelia Huck <cohuck@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: Stefan Hajnoczi <stefanha@redhat.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Anthony Perard <anthony.perard@citrix.com> CC: Paul Durrant <paul@xen.org> 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 | 84 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-)