| Message ID | 20200306051536.27803-2-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | error: auto propagated local_err part I | expand |
On Fri, 6 Mar 2020 at 05:16, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > 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: Paul Durrant <paul@xen.org>
On Fri, 6 Mar 2020 08:15:27 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 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> > --- > Thanks for this impressive work Vladimir ! Reviewed-by: Greg Kurz <groug@kaod.org> > 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: Christian Schoenebeck <qemu_oss@crudebyte.com> > 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: qemu-devel@nongnu.org > Cc: xen-devel@lists.xenproject.org > > include/qapi/error.h | 203 ++++++++++++++++++++++++++++++++++++------- > 1 file changed, 170 insertions(+), 33 deletions(-) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index ad5b6e896d..bb9bcf02fb 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -15,6 +15,8 @@ > /* > * Error reporting system loosely patterned after Glib's GError. > * > + * = Deal with Error object = > + * > * Create an error: > * error_setg(&err, "situation normal, all fouled up"); > * > @@ -47,28 +49,88 @@ > * reporting it (primarily useful in testsuites): > * error_free_or_abort(&err); > * > - * Pass an existing error to the caller: > - * error_propagate(errp, err); > - * where Error **errp is a parameter, by convention the last one. > + * = Deal with Error ** function parameter = > * > - * Pass an existing error to the caller with the message modified: > - * error_propagate_prepend(errp, err); > + * Function may use error system to return errors. In this case function > + * defines Error **errp parameter, which should be the last one (except for > + * functions which varidic argument list), which has the following API: > * > - * Avoid > - * error_propagate(errp, err); > - * error_prepend(errp, "Could not frobnicate '%s': ", name); > - * because this fails to prepend when @errp is &error_fatal. > + * Caller may pass as errp: > + * 1. &error_abort > + * This means abort on any error > + * 2. &error_fatal > + * Exit with non-zero return code on error > + * 3. NULL > + * Ignore errors > + * 4. Another value > + * On error allocate error object and set errp > * > - * Create a new error and pass it to the caller: > - * error_setg(errp, "situation normal, all fouled up"); > + * Error API functions with Error ** (like error_setg) argument supports these > + * rules, so user functions just need to use them appropriately (read below). > * > - * Call a function and receive an error from it: > + * Simple pass error to the caller: > + * error_setg(errp, "Some error"); > + * > + * Subcall of another errp-based function, passing the error to the caller > + * f(..., errp); > + * > + * == Checking success of subcall == > + * > + * If function returns error code in addition to errp (which is recommended), > + * you don't need any additional code, just do: > + * int ret = f(..., errp); > + * if (ret < 0) { > + * ... handle error ... > + * return ret; > + * } > + * > + * If function returns nothing (which is not recommended API) and the only way > + * to check success is checking errp, we must care about cases [1-3] above. We > + * need to use macro ERRP_AUTO_PROPAGATE (see below for details) like this: > + * > + * int our_func(..., Error **errp) { > + * ERRP_AUTO_PROPAGATE(); > + * ... > + * subcall(..., errp); > + * if (*errp) { > + * ... > + * return -ERRNO; > + * } > + * ... > + * } > + * > + * ERRP_AUTO_PROPAGATE cares about Error ** API, wraps original errp if needed, > + * so that it can be safely used (including dereferencing), and auto-propagates > + * error to original errp on function end. > + * > + * In some cases, we need to check result of subcall, but do not want to > + * propagate the Error object to our caller. In such cases we don't need > + * ERRP_AUTO_PROPAGATE, but just a local Error object: > + * > + * Receive an error and not pass it: > * Error *err = NULL; > - * foo(arg, &err); > + * subcall(arg, &err); > * if (err) { > * handle the error... > + * error_free(err); > * } > * > + * Note, that before ERRP_AUTO_PROPAGATE introduction the pattern above (with > + * error_propagate() instead of error_free()) was used to check and pass error > + * to the caller. Now this is DEPRECATED* (see below). > + * > + * Note also, that if you want to use error_append_hint/error_prepend or their > + * variants, you must use ERRP_AUTO_PROPAGATE too. Otherwise, in case of > + * error_fatal, you'll miss the chance to insert your additional information > + * into Error object. > + * > + * In rare cases, we need to pass existing Error object to the caller by hand: > + * error_propagate(errp, err); > + * > + * Pass an existing error to the caller with the message modified: > + * error_propagate_prepend(errp, err); > + * > + * > * Call a function ignoring errors: > * foo(arg, NULL); > * > @@ -78,26 +140,6 @@ > * Call a function treating errors as fatal: > * foo(arg, &error_fatal); > * > - * Receive an error and pass it on to the caller: > - * Error *err = NULL; > - * foo(arg, &err); > - * if (err) { > - * handle the error... > - * error_propagate(errp, err); > - * } > - * where Error **errp is a parameter, by convention the last one. > - * > - * Do *not* "optimize" this to > - * foo(arg, errp); > - * if (*errp) { // WRONG! > - * handle the error... > - * } > - * because errp may be NULL! > - * > - * But when all you do with the error is pass it on, please use > - * foo(arg, errp); > - * for readability. > - * > * Receive and accumulate multiple errors (first one wins): > * Error *err = NULL, *local_err = NULL; > * foo(arg, &err); > @@ -114,6 +156,61 @@ > * handle the error... > * } > * because this may pass a non-null err to bar(). > + * > + * DEPRECATED* > + * > + * The following pattern of receiving checking and passing the caller of the > + * error by hand is deprecated now: > + * > + * Error *err = NULL; > + * foo(arg, &err); > + * if (err) { > + * handle the error... > + * error_propagate(errp, err); > + * } > + * > + * Instead, use ERRP_AUTO_PROPAGATE macro (defined below). > + * > + * The old pattern is 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; > + * } > + * ... > + * } > */ > > #ifndef ERROR_H > @@ -322,6 +419,46 @@ 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}; \ > + do { \ > + if (!errp || errp == &error_fatal) { \ > + errp = &_auto_errp_prop.local_err; \ > + } \ > + } while (0) > + > /* > * Special error destination to abort on error. > * See error_setg() and error_propagate() for details.
On Fri 06 Mar 2020 06:15:27 AM CET, Vladimir Sementsov-Ogievskiy wrote: Sorry I just gave a quick look at these patches and noticed this: > + * Function may use error system to return errors. In this case function > + * defines Error **errp parameter, which should be the last one (except for > + * functions which varidic argument list), which has the following API: I don't think that "functions which varidic argument list" is correct English. Perhaps something like "except for variadic functions" or "except for functions with a variable number of arguments". Berto
On 3/5/20 11:15 PM, Vladimir Sementsov-Ogievskiy wrote: > Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of > functions with an errp OUT parameter. As an aid to writing imperative-style commit messages, I like to prepend an implicit "Apply this patch to..." before the user's text, to see if things still make sense. By that construct, this paragraph might read better as: Introduce a new ERRP_AUTO_PROPAGATE macro, ... > > 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> > --- I have lots of grammar suggestions for the comments (and I know Markus is generally okay doing wording tweaks, so it may still end up different than my suggestions): > +++ b/include/qapi/error.h > @@ -15,6 +15,8 @@ > /* > * Error reporting system loosely patterned after Glib's GError. > * > + * = Deal with Error object = > + * > * Create an error: > * error_setg(&err, "situation normal, all fouled up"); > * > @@ -47,28 +49,88 @@ > * reporting it (primarily useful in testsuites): > * error_free_or_abort(&err); > * > - * Pass an existing error to the caller: > - * error_propagate(errp, err); > - * where Error **errp is a parameter, by convention the last one. > + * = Deal with Error ** function parameter = > * > - * Pass an existing error to the caller with the message modified: > - * error_propagate_prepend(errp, err); > + * Function may use error system to return errors. In this case function > + * defines Error **errp parameter, which should be the last one (except for > + * functions which varidic argument list), which has the following API: A function may use the error system to return errors. In this case, the function defines an Error **errp parameter, by convention the last one (with exceptions for functions using ... or va_list). > * > - * Avoid > - * error_propagate(errp, err); > - * error_prepend(errp, "Could not frobnicate '%s': ", name); > - * because this fails to prepend when @errp is &error_fatal. > + * Caller may pass as errp: The caller may then pass in the following errp values: > + * 1. &error_abort > + * This means abort on any error Any error will result in abort() > + * 2. &error_fatal > + * Exit with non-zero return code on error Any error will result in exit() with a non-zero status > + * 3. NULL > + * Ignore errors Any error will be ignored > + * 4. Another value 4. The address of a NULL-initialized Error *err > + * On error allocate error object and set errp Any error will populate errp with an error object > * > - * Create a new error and pass it to the caller: > - * error_setg(errp, "situation normal, all fouled up"); > + * Error API functions with Error ** (like error_setg) argument supports these > + * rules, so user functions just need to use them appropriately (read below). The following rules then implement the correct semantics desired by the caller. > * > - * Call a function and receive an error from it: > + * Simple pass error to the caller: Create a new error to pass to the caller: > + * error_setg(errp, "Some error"); You lost the fun wording in Markus' earlier example ("situation normal, all fouled up"). > + * > + * Subcall of another errp-based function, passing the error to the caller > + * f(..., errp); Calling another errp-based function: > + * > + * == Checking success of subcall == > + * > + * If function returns error code in addition to errp (which is recommended), If a function returns a value indicating an error in addition to setting errp (which is recommended), then > + * you don't need any additional code, just do: > + * int ret = f(..., errp); > + * if (ret < 0) { > + * ... handle error ... > + * return ret; > + * } > + * > + * If function returns nothing (which is not recommended API) and the only way > + * to check success is checking errp, we must care about cases [1-3] above. We > + * need to use macro ERRP_AUTO_PROPAGATE (see below for details) like this: If a function returns nothing (not recommended for new code), the only way to check success is by consulting errp; doing this safely requires the use of the ERRP_AUTO_PROPAGATE macro, like this: > + * > + * int our_func(..., Error **errp) { > + * ERRP_AUTO_PROPAGATE(); > + * ... > + * subcall(..., errp); > + * if (*errp) { > + * ... > + * return -ERRNO; do we want ERRNO capitalized here? > + * } > + * ... > + * } > + * > + * ERRP_AUTO_PROPAGATE cares about Error ** API, wraps original errp if needed, > + * so that it can be safely used (including dereferencing), and auto-propagates > + * error to original errp on function end. ERRP_AUTO_PROPAGATE takes care of wrapping the original errp as needed, so that the rest of the function can directly use errp (including dereferencing), where any errors will then be propagated on to the original errp when leaving the function. > + * > + * In some cases, we need to check result of subcall, but do not want to > + * propagate the Error object to our caller. In such cases we don't need > + * ERRP_AUTO_PROPAGATE, but just a local Error object: > + * > + * Receive an error and not pass it: > * Error *err = NULL; > - * foo(arg, &err); > + * subcall(arg, &err); > * if (err) { > * handle the error... > + * error_free(err); > * } > * > + * Note, that before ERRP_AUTO_PROPAGATE introduction the pattern above (with > + * error_propagate() instead of error_free()) was used to check and pass error > + * to the caller. Now this is DEPRECATED* (see below). Hmm - if we bulk-convert the entire tree, then there won't be any deprecated uses to be worth documenting. But if we do keep this paragraph: Note that older code that did not use ERRP_AUTO_PROPAGATE would instead need a local Err variable and the use of error_propagate() to properly handle all possible caller values of errp. > + * > + * Note also, that if you want to use error_append_hint/error_prepend or their > + * variants, you must use ERRP_AUTO_PROPAGATE too. Otherwise, in case of > + * error_fatal, you'll miss the chance to insert your additional information > + * into Error object. Note that any function that wants to modify an error object, such as by calling error_append_hint or error_prepend, must use ERRP_AUTO_PROPAGATE, in order for a caller's use of &error_fatal to see the additional information. > + * > + * In rare cases, we need to pass existing Error object to the caller by hand: > + * error_propagate(errp, err); > + * > + * Pass an existing error to the caller with the message modified: > + * error_propagate_prepend(errp, err); > + * > + * > * Call a function ignoring errors: > * foo(arg, NULL); > * > @@ -78,26 +140,6 @@ > * Call a function treating errors as fatal: > * foo(arg, &error_fatal); > * > - * Receive an error and pass it on to the caller: > - * Error *err = NULL; > - * foo(arg, &err); > - * if (err) { > - * handle the error... > - * error_propagate(errp, err); > - * } > - * where Error **errp is a parameter, by convention the last one. > - * > - * Do *not* "optimize" this to > - * foo(arg, errp); > - * if (*errp) { // WRONG! > - * handle the error... > - * } > - * because errp may be NULL! > - * > - * But when all you do with the error is pass it on, please use > - * foo(arg, errp); > - * for readability. > - * > * Receive and accumulate multiple errors (first one wins): > * Error *err = NULL, *local_err = NULL; > * foo(arg, &err); > @@ -114,6 +156,61 @@ > * handle the error... > * } > * because this may pass a non-null err to bar(). > + * > + * DEPRECATED* > + * Again, I'm not sure we need this section in the codebase if we do a bulk conversion. But moving it to the commit message is still useful. > + * The following pattern of receiving checking and passing the caller of the > + * error by hand is deprecated now: The following pattern of receiving, checking, and then forwarding an error to the caller by hand is now deprecated: > + * > + * Error *err = NULL; > + * foo(arg, &err); > + * if (err) { > + * handle the error... > + * error_propagate(errp, err); > + * } > + * > + * Instead, use ERRP_AUTO_PROPAGATE macro (defined below). Drop "(defined below)". > + * > + * The old pattern is 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; > + * } > + * ... > + * } Again, I'm thinking the above example is more useful in the commit message instead of permanently in the .h file. > */ > > #ifndef ERROR_H > @@ -322,6 +419,46 @@ 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. This macro exists to assist with proper error handling in a function which uses an Error **errp parameter. It must be used as the first line of a function which modifies an error (with error_prepend, error_append_hint, or similar) or which wants to dereference *errp. It is still safe (but useless) to use in other functions. > + * > + * 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}; \ > + do { \ > + if (!errp || errp == &error_fatal) { \ > + errp = &_auto_errp_prop.local_err; \ > + } \ > + } while (0) > + > /* > * Special error destination to abort on error. > * See error_setg() and error_propagate() for details. > The macro itself looks correct. I'll leave it up to Markus how to handle the comment text, but you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
06.03.2020 15:37, Eric Blake wrote: > On 3/5/20 11:15 PM, Vladimir Sementsov-Ogievskiy wrote: >> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of >> functions with an errp OUT parameter. > > As an aid to writing imperative-style commit messages, I like to prepend an implicit "Apply this patch to..." before the user's text, to see if things still make sense. By that construct, this paragraph might read better as: > > Introduce a new ERRP_AUTO_PROPAGATE macro, ... > >> >> 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> >> --- > > I have lots of grammar suggestions for the comments (and I know Markus is generally okay doing wording tweaks, so it may still end up different than my suggestions): > >> +++ b/include/qapi/error.h >> @@ -15,6 +15,8 @@ >> /* >> * Error reporting system loosely patterned after Glib's GError. >> * >> + * = Deal with Error object = >> + * >> * Create an error: >> * error_setg(&err, "situation normal, all fouled up"); >> * >> @@ -47,28 +49,88 @@ >> * reporting it (primarily useful in testsuites): >> * error_free_or_abort(&err); >> * >> - * Pass an existing error to the caller: >> - * error_propagate(errp, err); >> - * where Error **errp is a parameter, by convention the last one. >> + * = Deal with Error ** function parameter = >> * >> - * Pass an existing error to the caller with the message modified: >> - * error_propagate_prepend(errp, err); >> + * Function may use error system to return errors. In this case function >> + * defines Error **errp parameter, which should be the last one (except for >> + * functions which varidic argument list), which has the following API: > > A function may use the error system to return errors. In this case, the function defines an Error **errp parameter, by convention the last one (with exceptions for functions using ... or va_list). > >> * >> - * Avoid >> - * error_propagate(errp, err); >> - * error_prepend(errp, "Could not frobnicate '%s': ", name); >> - * because this fails to prepend when @errp is &error_fatal. >> + * Caller may pass as errp: > > The caller may then pass in the following errp values: > >> + * 1. &error_abort >> + * This means abort on any error > > Any error will result in abort() > >> + * 2. &error_fatal >> + * Exit with non-zero return code on error > > Any error will result in exit() with a non-zero status > >> + * 3. NULL >> + * Ignore errors > > Any error will be ignored > >> + * 4. Another value > > 4. The address of a NULL-initialized Error *err > >> + * On error allocate error object and set errp > > Any error will populate errp with an error object > >> * >> - * Create a new error and pass it to the caller: >> - * error_setg(errp, "situation normal, all fouled up"); >> + * Error API functions with Error ** (like error_setg) argument supports these >> + * rules, so user functions just need to use them appropriately (read below). > > The following rules then implement the correct semantics desired by the caller. > >> * >> - * Call a function and receive an error from it: >> + * Simple pass error to the caller: > > Create a new error to pass to the caller: > >> + * error_setg(errp, "Some error"); > > You lost the fun wording in Markus' earlier example ("situation normal, all fouled up"). > >> + * >> + * Subcall of another errp-based function, passing the error to the caller >> + * f(..., errp); > > Calling another errp-based function: > >> + * >> + * == Checking success of subcall == >> + * >> + * If function returns error code in addition to errp (which is recommended), > > If a function returns a value indicating an error in addition to setting errp (which is recommended), then > >> + * you don't need any additional code, just do: >> + * int ret = f(..., errp); >> + * if (ret < 0) { >> + * ... handle error ... >> + * return ret; >> + * } >> + * >> + * If function returns nothing (which is not recommended API) and the only way >> + * to check success is checking errp, we must care about cases [1-3] above. We >> + * need to use macro ERRP_AUTO_PROPAGATE (see below for details) like this: > > If a function returns nothing (not recommended for new code), the only way to check success is by consulting errp; doing this safely requires the use of the ERRP_AUTO_PROPAGATE macro, like this: > > >> + * >> + * int our_func(..., Error **errp) { >> + * ERRP_AUTO_PROPAGATE(); >> + * ... >> + * subcall(..., errp); >> + * if (*errp) { >> + * ... >> + * return -ERRNO; > > do we want ERRNO capitalized here? I wrote it capitalized to not conflict with errno variable, to make it obvious that this is just a pseudo-code. But yes, it looks weird anyway. We can write -errno here, or just -EINVAL (I hope, nobady will thing, that EINVAL should always be used :) > >> + * } >> + * ... >> + * } >> + * >> + * ERRP_AUTO_PROPAGATE cares about Error ** API, wraps original errp if needed, >> + * so that it can be safely used (including dereferencing), and auto-propagates >> + * error to original errp on function end. > > ERRP_AUTO_PROPAGATE takes care of wrapping the original errp as needed, so that the rest of the function can directly use errp (including dereferencing), where any errors will then be propagated on to the original errp when leaving the function. > >> + * >> + * In some cases, we need to check result of subcall, but do not want to >> + * propagate the Error object to our caller. In such cases we don't need >> + * ERRP_AUTO_PROPAGATE, but just a local Error object: >> + * >> + * Receive an error and not pass it: >> * Error *err = NULL; >> - * foo(arg, &err); >> + * subcall(arg, &err); >> * if (err) { >> * handle the error... >> + * error_free(err); >> * } >> * >> + * Note, that before ERRP_AUTO_PROPAGATE introduction the pattern above (with >> + * error_propagate() instead of error_free()) was used to check and pass error >> + * to the caller. Now this is DEPRECATED* (see below). > > Hmm - if we bulk-convert the entire tree, then there won't be any deprecated uses to be worth documenting. Yes, I think we'll drop it after everything is converted. > But if we do keep this paragraph: > > Note that older code that did not use ERRP_AUTO_PROPAGATE would instead need a local Err variable and the use of error_propagate() to properly handle all possible caller values of errp. > >> + * >> + * Note also, that if you want to use error_append_hint/error_prepend or their >> + * variants, you must use ERRP_AUTO_PROPAGATE too. Otherwise, in case of >> + * error_fatal, you'll miss the chance to insert your additional information >> + * into Error object. > > Note that any function that wants to modify an error object, such as by calling error_append_hint or error_prepend, must use ERRP_AUTO_PROPAGATE, in order for a caller's use of &error_fatal to see the additional information. > >> + * >> + * In rare cases, we need to pass existing Error object to the caller by hand: >> + * error_propagate(errp, err); >> + * >> + * Pass an existing error to the caller with the message modified: >> + * error_propagate_prepend(errp, err); >> + * >> + * >> * Call a function ignoring errors: >> * foo(arg, NULL); >> * >> @@ -78,26 +140,6 @@ >> * Call a function treating errors as fatal: >> * foo(arg, &error_fatal); >> * >> - * Receive an error and pass it on to the caller: >> - * Error *err = NULL; >> - * foo(arg, &err); >> - * if (err) { >> - * handle the error... >> - * error_propagate(errp, err); >> - * } >> - * where Error **errp is a parameter, by convention the last one. >> - * >> - * Do *not* "optimize" this to >> - * foo(arg, errp); >> - * if (*errp) { // WRONG! >> - * handle the error... >> - * } >> - * because errp may be NULL! >> - * >> - * But when all you do with the error is pass it on, please use >> - * foo(arg, errp); >> - * for readability. >> - * >> * Receive and accumulate multiple errors (first one wins): >> * Error *err = NULL, *local_err = NULL; >> * foo(arg, &err); >> @@ -114,6 +156,61 @@ >> * handle the error... >> * } >> * because this may pass a non-null err to bar(). >> + * >> + * DEPRECATED* >> + * > > Again, I'm not sure we need this section in the codebase if we do a bulk conversion. But moving it to the commit message is still useful. Conversion can't be atomic anyway, there would be several series. But I'm not against dropping the paragraph, so, it's up to Markus. > >> + * The following pattern of receiving checking and passing the caller of the >> + * error by hand is deprecated now: > > The following pattern of receiving, checking, and then forwarding an error to the caller by hand is now deprecated: > >> + * >> + * Error *err = NULL; >> + * foo(arg, &err); >> + * if (err) { >> + * handle the error... >> + * error_propagate(errp, err); >> + * } >> + * >> + * Instead, use ERRP_AUTO_PROPAGATE macro (defined below). > > Drop "(defined below)". > >> + * >> + * The old pattern is 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; >> + * } >> + * ... >> + * } > > Again, I'm thinking the above example is more useful in the commit message instead of permanently in the .h file. > >> */ >> #ifndef ERROR_H >> @@ -322,6 +419,46 @@ 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. > > This macro exists to assist with proper error handling in a function which uses an Error **errp parameter. It must be used as the first line of a function which modifies an error (with error_prepend, error_append_hint, or similar) or which wants to dereference *errp. It is still safe (but useless) to use in other functions. > >> + * >> + * 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}; \ >> + do { \ >> + if (!errp || errp == &error_fatal) { \ >> + errp = &_auto_errp_prop.local_err; \ >> + } \ >> + } while (0) >> + >> /* >> * Special error destination to abort on error. >> * See error_setg() and error_propagate() for details. >> > > The macro itself looks correct. I'll leave it up to Markus how to handle the comment text, but you can add: > > Reviewed-by: Eric Blake <eblake@redhat.com> > Thanks for reviewing and wording suggestions, all looks good, as always.
diff --git a/include/qapi/error.h b/include/qapi/error.h index ad5b6e896d..bb9bcf02fb 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -15,6 +15,8 @@ /* * Error reporting system loosely patterned after Glib's GError. * + * = Deal with Error object = + * * Create an error: * error_setg(&err, "situation normal, all fouled up"); * @@ -47,28 +49,88 @@ * reporting it (primarily useful in testsuites): * error_free_or_abort(&err); * - * Pass an existing error to the caller: - * error_propagate(errp, err); - * where Error **errp is a parameter, by convention the last one. + * = Deal with Error ** function parameter = * - * Pass an existing error to the caller with the message modified: - * error_propagate_prepend(errp, err); + * Function may use error system to return errors. In this case function + * defines Error **errp parameter, which should be the last one (except for + * functions which varidic argument list), which has the following API: * - * Avoid - * error_propagate(errp, err); - * error_prepend(errp, "Could not frobnicate '%s': ", name); - * because this fails to prepend when @errp is &error_fatal. + * Caller may pass as errp: + * 1. &error_abort + * This means abort on any error + * 2. &error_fatal + * Exit with non-zero return code on error + * 3. NULL + * Ignore errors + * 4. Another value + * On error allocate error object and set errp * - * Create a new error and pass it to the caller: - * error_setg(errp, "situation normal, all fouled up"); + * Error API functions with Error ** (like error_setg) argument supports these + * rules, so user functions just need to use them appropriately (read below). * - * Call a function and receive an error from it: + * Simple pass error to the caller: + * error_setg(errp, "Some error"); + * + * Subcall of another errp-based function, passing the error to the caller + * f(..., errp); + * + * == Checking success of subcall == + * + * If function returns error code in addition to errp (which is recommended), + * you don't need any additional code, just do: + * int ret = f(..., errp); + * if (ret < 0) { + * ... handle error ... + * return ret; + * } + * + * If function returns nothing (which is not recommended API) and the only way + * to check success is checking errp, we must care about cases [1-3] above. We + * need to use macro ERRP_AUTO_PROPAGATE (see below for details) like this: + * + * int our_func(..., Error **errp) { + * ERRP_AUTO_PROPAGATE(); + * ... + * subcall(..., errp); + * if (*errp) { + * ... + * return -ERRNO; + * } + * ... + * } + * + * ERRP_AUTO_PROPAGATE cares about Error ** API, wraps original errp if needed, + * so that it can be safely used (including dereferencing), and auto-propagates + * error to original errp on function end. + * + * In some cases, we need to check result of subcall, but do not want to + * propagate the Error object to our caller. In such cases we don't need + * ERRP_AUTO_PROPAGATE, but just a local Error object: + * + * Receive an error and not pass it: * Error *err = NULL; - * foo(arg, &err); + * subcall(arg, &err); * if (err) { * handle the error... + * error_free(err); * } * + * Note, that before ERRP_AUTO_PROPAGATE introduction the pattern above (with + * error_propagate() instead of error_free()) was used to check and pass error + * to the caller. Now this is DEPRECATED* (see below). + * + * Note also, that if you want to use error_append_hint/error_prepend or their + * variants, you must use ERRP_AUTO_PROPAGATE too. Otherwise, in case of + * error_fatal, you'll miss the chance to insert your additional information + * into Error object. + * + * In rare cases, we need to pass existing Error object to the caller by hand: + * error_propagate(errp, err); + * + * Pass an existing error to the caller with the message modified: + * error_propagate_prepend(errp, err); + * + * * Call a function ignoring errors: * foo(arg, NULL); * @@ -78,26 +140,6 @@ * Call a function treating errors as fatal: * foo(arg, &error_fatal); * - * Receive an error and pass it on to the caller: - * Error *err = NULL; - * foo(arg, &err); - * if (err) { - * handle the error... - * error_propagate(errp, err); - * } - * where Error **errp is a parameter, by convention the last one. - * - * Do *not* "optimize" this to - * foo(arg, errp); - * if (*errp) { // WRONG! - * handle the error... - * } - * because errp may be NULL! - * - * But when all you do with the error is pass it on, please use - * foo(arg, errp); - * for readability. - * * Receive and accumulate multiple errors (first one wins): * Error *err = NULL, *local_err = NULL; * foo(arg, &err); @@ -114,6 +156,61 @@ * handle the error... * } * because this may pass a non-null err to bar(). + * + * DEPRECATED* + * + * The following pattern of receiving checking and passing the caller of the + * error by hand is deprecated now: + * + * Error *err = NULL; + * foo(arg, &err); + * if (err) { + * handle the error... + * error_propagate(errp, err); + * } + * + * Instead, use ERRP_AUTO_PROPAGATE macro (defined below). + * + * The old pattern is 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; + * } + * ... + * } */ #ifndef ERROR_H @@ -322,6 +419,46 @@ 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}; \ + do { \ + if (!errp || errp == &error_fatal) { \ + errp = &_auto_errp_prop.local_err; \ + } \ + } while (0) + /* * 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 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> --- 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: Christian Schoenebeck <qemu_oss@crudebyte.com> 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: qemu-devel@nongnu.org Cc: xen-devel@lists.xenproject.org include/qapi/error.h | 203 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 170 insertions(+), 33 deletions(-)