19.09.2019 10:32, David Hildenbrand wrote: > On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote: >> Hi all! >> >> Here is a proposal (three of them, actually) of auto propagation for >> local_err, to not call error_propagate on every exit point, when we >> deal with local_err. >> >> It also may help make Greg's series[1] about error_append_hint smaller. >> >> See definitions and examples below. >> >> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like >> it, the idea will touch same code (and may be more). >> >> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> --- >> include/qapi/error.h | 102 +++++++++++++++++++++++++++++++++++++++++++ >> block.c | 63 ++++++++++++-------------- >> block/backup.c | 8 +++- >> block/gluster.c | 7 +++ >> 4 files changed, 144 insertions(+), 36 deletions(-) >> >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index 3f95141a01..083e061014 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp, >> ErrorClass err_class, const char *fmt, ...) >> GCC_FMT_ATTR(6, 7); >> >> +typedef struct ErrorPropagator { >> + Error **errp; >> + Error *local_err; >> +} ErrorPropagator; >> + >> +static inline void error_propagator_cleanup(ErrorPropagator *prop) >> +{ >> + if (prop->local_err) { >> + error_propagate(prop->errp, prop->local_err); >> + } >> +} >> + >> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup); >> + >> +/* >> + * ErrorPropagationPair >> + * >> + * [Error *local_err, Error **errp] >> + * >> + * First element is local_err, second is original errp, which is propagation >> + * target. Yes, errp has a bit another type, so it should be converted. >> + * >> + * ErrorPropagationPair may be used as errp, which points to local_err, >> + * as it's type is compatible. >> + */ >> +typedef Error *ErrorPropagationPair[2]; >> + >> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr) >> +{ >> + Error *local_err = (*arr)[0]; >> + Error **errp = (Error **)(*arr)[1]; >> + >> + if (local_err) { >> + error_propagate(errp, local_err); >> + } >> +} >> + >> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair, >> + error_propagation_pair_cleanup); >> + >> +/* >> + * DEF_AUTO_ERRP >> + * >> + * Define auto_errp variable, which may be used instead of errp, and >> + * *auto_errp may be safely checked to be zero or not, and may be safely >> + * used for error_append_hint(). auto_errp is automatically propagated >> + * to errp at function exit. >> + */ >> +#define DEF_AUTO_ERRP(auto_errp, errp) \ >> + g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)} >> + >> + >> +/* >> + * Another variant: >> + * Pros: >> + * - normal structure instead of cheating with array >> + * - we can directly use errp, if it's not NULL and don't point to >> + * error_abort or error_fatal >> + * Cons: >> + * - we need to define two variables instead of one >> + */ >> +typedef struct ErrorPropagationStruct { >> + Error *local_err; >> + Error **errp; >> +} ErrorPropagationStruct; >> + >> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct >> *prop) >> +{ >> + if (prop->local_err) { >> + error_propagate(prop->errp, prop->local_err); >> + } >> +} >> + >> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct, >> + error_propagation_struct_cleanup); >> + >> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \ >> + g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \ >> + Error **auto_errp = \ >> + ((errp) == NULL || *(errp) == error_abort || *(errp) == >> error_fatal) ? \ >> + &__auto_errp_prop.local_err : \ >> + (errp); >> + >> +/* >> + * Third variant: >> + * Pros: >> + * - simpler movement for functions which don't have local_err yet >> + * the only thing to do is to call one macro at function start. >> + * This extremely simplifies Greg's series >> + * Cons: >> + * - looks like errp shadowing.. Still seems safe. >> + * - must be after all definitions of local variables and before any >> + * code. >> + * - like v2, several statements in one open macro >> + */ >> +#define MAKE_ERRP_SAFE(errp) \ >> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \ >> +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) { \ >> + (errp) = &__auto_errp_prop.local_err; \ >> +} > > > Using that idea, what about something like this: > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 8bfb6684cb..043ad69f8b 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu); > } > > +typedef struct ErrorPropagator { > + Error **errp; > + Error *local_err; > +} ErrorPropagator; > + > +static inline void error_propagator_cleanup(ErrorPropagator *prop) > +{ > + if (prop->local_err) { > + error_propagate(prop->errp, prop->local_err); > + } > +} > + > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup); > + > +#define DEFINE_LOCAL_ERRP(_errp) \ > +g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = (_errp)}; \ > +Error **local_errp = &__auto_errp_prop.local_err > + > static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id, > Error **errp) > { > + DEFINE_LOCAL_ERRP(errp); > S390CPU *cpu = S390_CPU(object_new(typename)); > - Error *err = NULL; > > - object_property_set_int(OBJECT(cpu), core_id, "core-id", &err); > - if (err != NULL) { > + object_property_set_int(OBJECT(cpu), core_id, "core-id", local_errp); > + if (*local_errp != NULL) { > goto out; > } > - object_property_set_bool(OBJECT(cpu), true, "realized", &err); > + object_property_set_bool(OBJECT(cpu), true, "realized", local_errp); > > out: > object_unref(OBJECT(cpu)); > - if (err) { > - error_propagate(errp, err); > + if (*local_errp) { > cpu = NULL; > } > return cpu; > >
So it's DEF_AUTO_ERRP_V2 with first parameter hardcoded to be local_errp. I still prefer MAKE_ERRP_SAFE(), to not introduce extra variables. -- Best regards, Vladimir
