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; -- Thanks, David / dhildenb