On 19.09.19 10:20, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:53, David Hildenbrand wrote:
>> 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.
>>
>
> But it makes Greg's series extremely simple: just add MAKE_ERRP_SAFE() to some
> functions. And as Eric explains, mixing code and definitions is not a problem.
I still dislike it ;)
>
> Still, we can do like this:
>
> #define MAKE_ERRP_SAFE() \
> g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = errp}; \
> Error **__local_errp_unused __attribute__ ((unused)) = (errp =
> &__auto_errp_prop.local_err)
>
> Which are two valid definitions.
Yeah, I would prefer something like that!
--
Thanks,
David / dhildenb
