| Message ID | 20190924200902.4703-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: auto propagated local_err | expand |
On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote: > Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of > functions with errp parameter. > > It has three goals: > > 1. Fix issue with error_fatal & error_append_hint: user can't see these > hints, because exit() happens in error_setg earlier than hint is > appended. [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> > --- > > +/* > + * ERRP_FUNCTION_BEGIN > + * > + * This macro is created to be the first line of a function with Error **errp > + * parameter. Since you're going with the reduced scope of only touching functions with error_append_hint, we may want to tweak this sentence to call out only the functions that REQUIRE the use of this macro (if they contain *errp or error_apend_hint), while mentioning that it is still safe to use on any function with an errp parameter. I'm hoping Markus will be able to chime in with his preferences soon. > + * > + * 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_FUNCTION_BEGIN() \ > +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \ > +errp = ((errp == NULL || *errp == error_fatal) ? \ > + &__auto_errp_prop.local_err : errp) > + Reviewed-by: Eric Blake <eblake@redhat.com>
Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: > Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of > functions with errp parameter. A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will follow. Can we find a different name, especially now that we won't use this macro in every function that uses an errp, so even the "errp function" part isn't really correct any more? How about ERRP_AUTO_PROPAGATE? Kevin
30.09.2019 18:12, Kevin Wolf wrote: > Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of >> functions with errp parameter. > > A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will > follow. Can we find a different name, especially now that we won't use > this macro in every function that uses an errp, so even the "errp > function" part isn't really correct any more? > > How about ERRP_AUTO_PROPAGATE? > I have an idea that with this macro we can (optionally) get the whole call stack of the error and print it to log, so it's good to give it more generic name, not limited to propagation.. WRAP_ERRP AUTO_ERRP AUTOMATE_ERRP or what do you think?
Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben: > 30.09.2019 18:12, Kevin Wolf wrote: > > Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: > >> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of > >> functions with errp parameter. > > > > A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will > > follow. Can we find a different name, especially now that we won't use > > this macro in every function that uses an errp, so even the "errp > > function" part isn't really correct any more? > > > > How about ERRP_AUTO_PROPAGATE? > > I have an idea that with this macro we can (optionally) get the whole call stack > of the error and print it to log, so it's good to give it more generic name, not > limited to propagation.. Hm, what's the context for this feature? The obvious one where you want to have a stack trace is &error_abort, but that one crashes, so you get it automatically. If it's just a normal error (like a QAPI option contains an invalid value and some function down the call chain checks it), why would anyone want to know what the call chain in the QEMU code was? Kevin
30.09.2019 19:00, Kevin Wolf wrote: > Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 30.09.2019 18:12, Kevin Wolf wrote: >>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of >>>> functions with errp parameter. >>> >>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will >>> follow. Can we find a different name, especially now that we won't use >>> this macro in every function that uses an errp, so even the "errp >>> function" part isn't really correct any more? >>> >>> How about ERRP_AUTO_PROPAGATE? >> >> I have an idea that with this macro we can (optionally) get the whole call stack >> of the error and print it to log, so it's good to give it more generic name, not >> limited to propagation.. > > Hm, what's the context for this feature? > > The obvious one where you want to have a stack trace is &error_abort, > but that one crashes, so you get it automatically. If it's just a normal > error (like a QAPI option contains an invalid value and some function > down the call chain checks it), why would anyone want to know what the > call chain in the QEMU code was? > When I have bug from testers, call stack would be a lot more descriptive, than just an error message. We may add trace point which will print this information, so with disabled trace point - no extra output.
Am 30.09.2019 um 18:26 hat Vladimir Sementsov-Ogievskiy geschrieben: > 30.09.2019 19:00, Kevin Wolf wrote: > > Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben: > >> 30.09.2019 18:12, Kevin Wolf wrote: > >>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: > >>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of > >>>> functions with errp parameter. > >>> > >>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will > >>> follow. Can we find a different name, especially now that we won't use > >>> this macro in every function that uses an errp, so even the "errp > >>> function" part isn't really correct any more? > >>> > >>> How about ERRP_AUTO_PROPAGATE? > >> > >> I have an idea that with this macro we can (optionally) get the whole call stack > >> of the error and print it to log, so it's good to give it more generic name, not > >> limited to propagation.. > > > > Hm, what's the context for this feature? > > > > The obvious one where you want to have a stack trace is &error_abort, > > but that one crashes, so you get it automatically. If it's just a normal > > error (like a QAPI option contains an invalid value and some function > > down the call chain checks it), why would anyone want to know what the > > call chain in the QEMU code was? > > > > When I have bug from testers, call stack would be a lot more descriptive, than just > an error message. > > We may add trace point which will print this information, so with disabled trace point > - no extra output. But wouldn't it make much more sense then to optionally add this functionality to any trace point? I really don't see how this is related specifically to user-visible error messages. However, even if we decide that we want to have this in Error objects, wouldn't it make much more sense to use the real C stack trace and save it from the innermost error_set() using backtrace() or compiler built-ins rather than relying on an error_propagate() chain? Kevin
30.09.2019 19:39, Kevin Wolf wrote: > Am 30.09.2019 um 18:26 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 30.09.2019 19:00, Kevin Wolf wrote: >>> Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 30.09.2019 18:12, Kevin Wolf wrote: >>>>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of >>>>>> functions with errp parameter. >>>>> >>>>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will >>>>> follow. Can we find a different name, especially now that we won't use >>>>> this macro in every function that uses an errp, so even the "errp >>>>> function" part isn't really correct any more? >>>>> >>>>> How about ERRP_AUTO_PROPAGATE? >>>> >>>> I have an idea that with this macro we can (optionally) get the whole call stack >>>> of the error and print it to log, so it's good to give it more generic name, not >>>> limited to propagation.. >>> >>> Hm, what's the context for this feature? >>> >>> The obvious one where you want to have a stack trace is &error_abort, >>> but that one crashes, so you get it automatically. If it's just a normal >>> error (like a QAPI option contains an invalid value and some function >>> down the call chain checks it), why would anyone want to know what the >>> call chain in the QEMU code was? >>> >> >> When I have bug from testers, call stack would be a lot more descriptive, than just >> an error message. >> >> We may add trace point which will print this information, so with disabled trace point >> - no extra output. > > But wouldn't it make much more sense then to optionally add this > functionality to any trace point? I really don't see how this is related > specifically to user-visible error messages. Interesting idea > > However, even if we decide that we want to have this in Error objects, > wouldn't it make much more sense to use the real C stack trace and save > it from the innermost error_set() using backtrace() or compiler > built-ins rather than relying on an error_propagate() chain? > Hmm, I thought about this.. And in concatenation with the fact that we'll have macro not everywhere, backtrace may be better.. On the other hand, backtrace will not show coroutine entries.. OK, anyway, if we will track some additional information in trace-events or in macros or in error_* API functions, it's not bad to track some additional information in macro named ERRP_AUTO_PROPAGATE.
Am 01.10.2019 um 10:39 hat Vladimir Sementsov-Ogievskiy geschrieben: > 30.09.2019 19:39, Kevin Wolf wrote: > > Am 30.09.2019 um 18:26 hat Vladimir Sementsov-Ogievskiy geschrieben: > >> 30.09.2019 19:00, Kevin Wolf wrote: > >>> Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben: > >>>> 30.09.2019 18:12, Kevin Wolf wrote: > >>>>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben: > >>>>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of > >>>>>> functions with errp parameter. > >>>>> > >>>>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will > >>>>> follow. Can we find a different name, especially now that we won't use > >>>>> this macro in every function that uses an errp, so even the "errp > >>>>> function" part isn't really correct any more? > >>>>> > >>>>> How about ERRP_AUTO_PROPAGATE? > >>>> > >>>> I have an idea that with this macro we can (optionally) get the whole call stack > >>>> of the error and print it to log, so it's good to give it more generic name, not > >>>> limited to propagation.. > >>> > >>> Hm, what's the context for this feature? > >>> > >>> The obvious one where you want to have a stack trace is &error_abort, > >>> but that one crashes, so you get it automatically. If it's just a normal > >>> error (like a QAPI option contains an invalid value and some function > >>> down the call chain checks it), why would anyone want to know what the > >>> call chain in the QEMU code was? > >>> > >> > >> When I have bug from testers, call stack would be a lot more descriptive, than just > >> an error message. > >> > >> We may add trace point which will print this information, so with disabled trace point > >> - no extra output. > > > > But wouldn't it make much more sense then to optionally add this > > functionality to any trace point? I really don't see how this is related > > specifically to user-visible error messages. > > Interesting idea > > > > > However, even if we decide that we want to have this in Error objects, > > wouldn't it make much more sense to use the real C stack trace and save > > it from the innermost error_set() using backtrace() or compiler > > built-ins rather than relying on an error_propagate() chain? > > > Hmm, I thought about this.. And in concatenation with the fact that > we'll have macro not everywhere, backtrace may be better.. > > On the other hand, backtrace will not show coroutine entries.. Hm, good point. I wonder if we can easily get a stack trace not starting at the current point, but from a jmp_buf. Then we could just switch to the coroutine caller whenever we reach coroutine_trampoline(). But glibc doesn't seem to support this case easily, so that might mean rewriting all of the stack unwinding inside QEMU... Maybe not then. > OK, anyway, if we will track some additional information in > trace-events or in macros or in error_* API functions, it's not bad to > track some additional information in macro named ERRP_AUTO_PROPAGATE. Yes, I think tracking the information where we use ERRP_AUTO_PROPAGATE anyway is okay. I just wouldn't add the macro everywhere just for the sake of the additional information. Kevin
diff --git a/include/qapi/error.h b/include/qapi/error.h index 9376f59c35..fb41f7a790 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -322,6 +322,41 @@ 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_FUNCTION_BEGIN + * + * This macro is created to be the first line of a function with Error **errp + * parameter. + * + * 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_FUNCTION_BEGIN() \ +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_FUNCTION_BEGIN macro, to be used at start of functions with errp parameter. It has three goals: 1. Fix issue with error_fatal & error_append_hint: user can't see these hints, because exit() happens in error_setg earlier than hint is appended. [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: John Snow <jsnow@redhat.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Max Reitz <mreitz@redhat.com> CC: Fam Zheng <fam@euphon.net> CC: Jeff Cody <codyprime@gmail.com> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Greg Kurz <groug@kaod.org> CC: Subbaraya Sundeep <sundeep.lkml@gmail.com> CC: Peter Maydell <peter.maydell@linaro.org> CC: Paul Burton <pburton@wavecomp.com> CC: Aleksandar Rikalo <arikalo@wavecomp.com> CC: "Michael S. Tsirkin" <mst@redhat.com> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> CC: David Gibson <david@gibson.dropbear.id.au> CC: Yuval Shaia <yuval.shaia@oracle.com> CC: Cornelia Huck <cohuck@redhat.com> CC: Eric Farman <farman@linux.ibm.com> CC: Richard Henderson <rth@twiddle.net> CC: David Hildenbrand <david@redhat.com> CC: Halil Pasic <pasic@linux.ibm.com> CC: Christian Borntraeger <borntraeger@de.ibm.com> CC: Gerd Hoffmann <kraxel@redhat.com> CC: Alex Williamson <alex.williamson@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Michael Roth <mdroth@linux.vnet.ibm.com> CC: Juan Quintela <quintela@redhat.com> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com> CC: Eric Blake <eblake@redhat.com> CC: Jason Wang <jasowang@redhat.com> CC: "Daniel P. Berrangé" <berrange@redhat.com> CC: Eduardo Habkost <ehabkost@redhat.com> CC: qemu-block@nongnu.org CC: qemu-devel@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 | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)