| Message ID | 20191001155319.8066-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: auto propagated local_err | expand |
On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy 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). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- I'm still hoping Markus gets some review time for this series with a chance to speak up (and that we haven't been causing you a lot of work which would all have to change if Markus has a different viewpoint). Reviewed-by: Eric Blake <eblake@redhat.com> > +/* > + * 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 append hints (by error_append_hint) Maybe: 'to append 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 Tue, Oct 01, 2019 at 06:52:52PM +0300, Vladimir Sementsov-Ogievskiy 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). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > > CC: kwolf@redhat.com > CC: mreitz@redhat.com > CC: jsnow@redhat.com > CC: fam@euphon.net > CC: sw@weilnetz.de > CC: codyprime@gmail.com > CC: marcandre.lureau@redhat.com > CC: pbonzini@redhat.com > CC: groug@kaod.org > CC: sundeep.lkml@gmail.com > CC: peter.maydell@linaro.org > CC: stefanha@redhat.com > CC: pburton@wavecomp.com > CC: arikalo@wavecomp.com > CC: berrange@redhat.com > CC: ehabkost@redhat.com > CC: david@gibson.dropbear.id.au > CC: clg@kaod.org > CC: mst@redhat.com > CC: marcel.apfelbaum@gmail.com > CC: mark.cave-ayland@ilande.co.uk > CC: yuval.shaia@oracle.com > CC: cohuck@redhat.com > CC: farman@linux.ibm.com > CC: rth@twiddle.net > CC: david@redhat.com > CC: pasic@linux.ibm.com > CC: borntraeger@de.ibm.com > CC: kraxel@redhat.com > CC: alex.williamson@redhat.com > CC: andrew@aj.id.au > CC: joel@jms.id.au > CC: eblake@redhat.com > CC: armbru@redhat.com > CC: mdroth@linux.vnet.ibm.com > CC: quintela@redhat.com > CC: dgilbert@redhat.com > CC: jasowang@redhat.com > CC: qemu-block@nongnu.org > CC: integration@gluster.org > CC: qemu-arm@nongnu.org > CC: qemu-ppc@nongnu.org > CC: qemu-s390x@nongnu.org > > include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 9376f59c35..02f967ac1d 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -322,6 +322,43 @@ 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 append hints (by 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) > + I guess it has been discussed but I couldn't find it, so: what's the reason for casting in stone the name of the function parameter, which isn't quite so obvious when you see this macro used in the code? IMO if the macro took errp as a parameter that would be easier to follow. Thanks, Roman.
On 10/2/19 5:15 AM, Roman Kagan wrote: > On Tue, Oct 01, 2019 at 06:52:52PM +0300, Vladimir Sementsov-Ogievskiy 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] >> >> +/* >> + * ERRP_AUTO_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) >> + > > I guess it has been discussed but I couldn't find it, so: what's the > reason for casting in stone the name of the function parameter, which > isn't quite so obvious when you see this macro used in the code? IMO > if the macro took errp as a parameter that would be easier to follow. It was discussed. Forcing a specific name of the 'Error **errp' has tradeoffs: Pro: possible to write a sed script to check for missing spots in the macros (in fact, my sed script found spots missed by Coccinelle when not using the correct --macro-header option) Pro: enforces uniform style Con: misses instances that have typos or otherwise used the wrong name Allowing the macro to take the name of the parameter: Pro/Con: More flexible (allows use in more place, but loses consistency) Pro: Coccinelle can still handle the conversion Con: using sed to check if Coccinelle missed anything is harder But opinions on how to paint the bikeshed are still welcome; it's easy enough to respin the series with the macro taking an argument if that is agreed to indeed be more legible.
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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. > (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). Starting with stating your goals is an excellent idea. But I'd love to next read a high-level description of how your patch achieves or enables achieving these goals. > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- [...] > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 9376f59c35..02f967ac1d 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -322,6 +322,43 @@ 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 append hints (by error_append_hint) > + * (as, if it was error_fatal, we swapped it with a local_error to be > + * propagated on cleanup). Well, appending hints was always safe, it just didn't work with &error_fatal. Don't worry about that now, I'll probably want to polish this contract comment a bit anyway, but later. > + * > + * 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}; \ Took me a second to realize: the macro works, because the initializer implicitly initializes .local_error = NULL. __auto_errp_prop is an identifier reserved for any use. I think we could use _auto_errp_prop, which is only reserved for use as identifiers with file scope in both the ordinary and tag name spaces. See ISO/IEC 9899:1999 7.1.3 Reserved identifiers. > +errp = ((errp == NULL || *errp == error_fatal) ? \ > + &__auto_errp_prop.local_err : errp) > + Please indent multi-line macros like elsewhere in this file: #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. To be honest, the cover letter left me a bit skeptical, but now I think you might be up to something. Let's see how the patches putting the macro to use come out.
On Tue, 08 Oct 2019 18:03:13 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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. > > (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). > > Starting with stating your goals is an excellent idea. But I'd love to > next read a high-level description of how your patch achieves or enables > achieving these goals. > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > --- > [...] > > diff --git a/include/qapi/error.h b/include/qapi/error.h > > index 9376f59c35..02f967ac1d 100644 > > --- a/include/qapi/error.h > > +++ b/include/qapi/error.h > > @@ -322,6 +322,43 @@ 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 append hints (by error_append_hint) > > + * (as, if it was error_fatal, we swapped it with a local_error to be > > + * propagated on cleanup). > > Well, appending hints was always safe, it just didn't work with > &error_fatal. Don't worry about that now, I'll probably want to polish > this contract comment a bit anyway, but later. > FWIW I've already posted this: Author: Greg Kurz <groug@kaod.org> Date: Mon Oct 7 15:45:46 2019 +0200 error: Update error_append_hint()'s documenation error_setg() and error_propagate(), as well as their variants, cause QEMU to terminate when called with &error_fatal or &error_abort. This prevents to add hints since error_append_hint() isn't even called in this case. It means that error_append_hint() should only be used with a local error object, and then propagate this local error to the caller. Document this in <qapi/error.h> . Signed-off-by: Greg Kurz <groug@kaod.org> Message-id: <156871563702.196432.5964411202152101367.stgit@bahia.lan> https://patchwork.ozlabs.org/patch/1163278/ > > + * > > + * 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}; \ > > Took me a second to realize: the macro works, because the initializer > implicitly initializes .local_error = NULL. > > __auto_errp_prop is an identifier reserved for any use. I think we > could use _auto_errp_prop, which is only reserved for use as identifiers > with file scope in both the ordinary and tag name spaces. See ISO/IEC > 9899:1999 7.1.3 Reserved identifiers. > > > +errp = ((errp == NULL || *errp == error_fatal) ? \ > > + &__auto_errp_prop.local_err : errp) > > + > > Please indent multi-line macros like elsewhere in this file: > > #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. > > To be honest, the cover letter left me a bit skeptical, but now I think > you might be up to something. Let's see how the patches putting the > macro to use come out.
Greg Kurz <groug@kaod.org> writes: > On Tue, 08 Oct 2019 18:03:13 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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. >> > (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). >> >> Starting with stating your goals is an excellent idea. But I'd love to >> next read a high-level description of how your patch achieves or enables >> achieving these goals. >> >> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> > --- >> [...] >> > diff --git a/include/qapi/error.h b/include/qapi/error.h >> > index 9376f59c35..02f967ac1d 100644 >> > --- a/include/qapi/error.h >> > +++ b/include/qapi/error.h >> > @@ -322,6 +322,43 @@ 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 append hints (by error_append_hint) >> > + * (as, if it was error_fatal, we swapped it with a local_error to be >> > + * propagated on cleanup). >> >> Well, appending hints was always safe, it just didn't work with >> &error_fatal. Don't worry about that now, I'll probably want to polish >> this contract comment a bit anyway, but later. >> > > FWIW I've already posted this: > > Author: Greg Kurz <groug@kaod.org> > Date: Mon Oct 7 15:45:46 2019 +0200 > > error: Update error_append_hint()'s documenation > > error_setg() and error_propagate(), as well as their variants, cause > QEMU to terminate when called with &error_fatal or &error_abort. This > prevents to add hints since error_append_hint() isn't even called in > this case. > > It means that error_append_hint() should only be used with a local > error object, and then propagate this local error to the caller. > > Document this in <qapi/error.h> . > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Message-id: <156871563702.196432.5964411202152101367.stgit@bahia.lan> > https://patchwork.ozlabs.org/patch/1163278/ I marked your series containing this patch for review. I decided to review this one first. I clearly need look at yours as well before I can advise on how to proceed. [...]
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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] Done in PATCH 07-31. > 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). Not done. Can you prototype this part? A few manually done examples would give us an idea how the complete solution would look like. A (semi-)automated complete conversion of a subsystem would additionally give us an idea how to actually do the conversion. We can discuss applying ERRP_AUTO_PROPAGATE() as a bug fix for 1., and leave 2. and 3. for later. Feels like a half-done job to me. We've got too many of those in the tree already. Dunno.
09.10.2019 11:04, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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] > > Done in PATCH 07-31. > >> 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). > > Not done. Can you prototype this part? A few manually done examples > would give us an idea how the complete solution would look like. A > (semi-)automated complete conversion of a subsystem would additionally > give us an idea how to actually do the conversion. > > We can discuss applying ERRP_AUTO_PROPAGATE() as a bug fix for 1., and > leave 2. and 3. for later. Feels like a half-done job to me. We've got > too many of those in the tree already. Dunno. > If update everything it's about 90-140 patches. The whole thing was done in "[RFC v2 0/9] error: auto propagated local_err" <20190923161231.22028-1-vsementsov@virtuozzo.com> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg05157.html - it was so huge, that I decided to postpone 2 and 3...
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 09.10.2019 11:04, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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] >> >> Done in PATCH 07-31. >> >>> 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). >> >> Not done. Can you prototype this part? A few manually done examples >> would give us an idea how the complete solution would look like. A >> (semi-)automated complete conversion of a subsystem would additionally >> give us an idea how to actually do the conversion. >> >> We can discuss applying ERRP_AUTO_PROPAGATE() as a bug fix for 1., and >> leave 2. and 3. for later. Feels like a half-done job to me. We've got >> too many of those in the tree already. Dunno. >> > > If update everything it's about 90-140 patches. > > The whole thing was done in "[RFC v2 0/9] error: auto propagated local_err" > <20190923161231.22028-1-vsementsov@virtuozzo.com> > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg05157.html > > - it was so huge, that I decided to postpone 2 and 3... Suggest to post unsplit patches as RFC. This lets us see how the macro gets used, and can also help us decide on a sensible split. The automatic split isn't quite right in places, and feels excessively fine-grained to me.
diff --git a/include/qapi/error.h b/include/qapi/error.h index 9376f59c35..02f967ac1d 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -322,6 +322,43 @@ 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 append hints (by 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). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- CC: kwolf@redhat.com CC: mreitz@redhat.com CC: jsnow@redhat.com CC: fam@euphon.net CC: sw@weilnetz.de CC: codyprime@gmail.com CC: marcandre.lureau@redhat.com CC: pbonzini@redhat.com CC: groug@kaod.org CC: sundeep.lkml@gmail.com CC: peter.maydell@linaro.org CC: stefanha@redhat.com CC: pburton@wavecomp.com CC: arikalo@wavecomp.com CC: berrange@redhat.com CC: ehabkost@redhat.com CC: david@gibson.dropbear.id.au CC: clg@kaod.org CC: mst@redhat.com CC: marcel.apfelbaum@gmail.com CC: mark.cave-ayland@ilande.co.uk CC: yuval.shaia@oracle.com CC: cohuck@redhat.com CC: farman@linux.ibm.com CC: rth@twiddle.net CC: david@redhat.com CC: pasic@linux.ibm.com CC: borntraeger@de.ibm.com CC: kraxel@redhat.com CC: alex.williamson@redhat.com CC: andrew@aj.id.au CC: joel@jms.id.au CC: eblake@redhat.com CC: armbru@redhat.com CC: mdroth@linux.vnet.ibm.com CC: quintela@redhat.com CC: dgilbert@redhat.com CC: jasowang@redhat.com CC: qemu-block@nongnu.org CC: integration@gluster.org CC: qemu-arm@nongnu.org CC: qemu-ppc@nongnu.org CC: qemu-s390x@nongnu.org include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)