Re: [qemu-s390x] [RFC] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   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