On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote: > 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. >
I lost track of the different approaches ;) The local variable will most probably optimized out by the compiler. I dislike MAKE_ERRP_SAFE(), as it mixes defining a new variable with code. -- Thanks, David / dhildenb