Greg Kurz <gr...@kaod.org> writes: > On Tue, 08 Oct 2019 18:03:13 +0200 > Markus Armbruster <arm...@redhat.com> wrote: > >> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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 <gr...@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 <gr...@kaod.org> > > Message-id: <156871563702.196432.5964411202152101367.st...@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. [...]