Re: [PATCH 01/11] qom: Reduce use of error_propagate()

[Vladimir Sementsov-Ogievskiy]

24.09.2021 12:04, Kevin Wolf wrote:
> ERRP_GUARD() makes debugging easier by making sure that &error_abort
> still fails at the real origin of the error instead of
> error_propagate().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qom/object.c            |  7 +++----
>   qom/object_interfaces.c | 17 ++++++-----------
>   2 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index e86cb05b84..6be710bc40 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1389,7 +1389,7 @@ bool object_property_get(Object *obj, const char *name, 
> Visitor *v,
>   bool object_property_set(Object *obj, const char *name, Visitor *v,
>                            Error **errp)
>   {
> -    Error *err = NULL;
> +    ERRP_GUARD();
>       ObjectProperty *prop = object_property_find_err(obj, name, errp);
>   
>       if (prop == NULL) {
> @@ -1400,9 +1400,8 @@ bool object_property_set(Object *obj, const char *name, 
> Visitor *v,
>           error_setg(errp, QERR_PERMISSION_DENIED);
>           return false;
>       }
> -    prop->set(obj, v, name, prop->opaque, &err);
> -    error_propagate(errp, err);
> -    return !err;
> +    prop->set(obj, v, name, prop->opaque, errp);
> +    return !*errp;
>   }

This is OK: prop->set doesn't return value, so we have to analyze errp to make 
our own return value.

>   
>   bool object_property_set_str(Object *obj, const char *name,
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ad9b56b59a..80691e88cd 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -45,26 +45,21 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
>   static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
>                                                Visitor *v, Error **errp)
>   {
> +    ERRP_GUARD();
>       const QDictEntry *e;
> -    Error *local_err = NULL;
>   
> -    if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) {
> -        goto out;
> +    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
> +        return;
>       }
>       for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> -        if (!object_property_set(obj, e->key, v, &local_err)) {
> +        if (!object_property_set(obj, e->key, v, errp)) {
>               break;
>           }
>       }
> -    if (!local_err) {
> -        visit_check_struct(v, &local_err);
> +    if (!*errp) {
> +        visit_check_struct(v, errp);
>       }
>       visit_end_struct(v, NULL);
> -
> -out:
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
>   }
>   
>   void object_set_properties_from_keyval(Object *obj, const QDict *qdict,

ERRP_GUARD() + dereferencing errp is good when we use functions which don't 
return any value. So we want to check is it fail or not and we have to analyze 
errp.

But that is not the case: all called functions follow modern recommendations of 
returning some value indicating failure together with setting errp.

I think, it's better not use ERRP_GUARD where it is not needed: in ideal world, 
all functions that may fail do return value, and we don't need neither error 
propagation, nor dereferencing errp. And don't need ERRP_GUARD.  ERRP_GUARD 
will be still needed to support error_prepend()/error_append() together with 
error_fatal, but again that's not the case.

Here I suggest something like this:

static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
                                             Visitor *v, Error **errp)
{
    const QDictEntry *e;
                                                                                 
    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
        return;
    }
    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
        if (!object_property_set(obj, e->key, v, errp)) {
            goto out;
        }
    }

    visit_check_struct(v, errp);

out:
    visit_end_struct(v, NULL);
}


--
Best regards,
Vladimir