Re: [PATCH 13/17] qom: Drop parameter @errp of object_property_add() & friends

[Eric Blake <eblake@redhat.com>]

On 4/28/20 11:34 AM, Markus Armbruster wrote:
> The only way object_property_add() can fail is when a property with
> the same name already exists.  Since our property names are all
> hardcoded, failure is a programming error, and the appropriate way to
> handle it is passing &error_abort.
> 
> Same for its variants, except for object_property_add_child(), which
> additionally fails when the child already has a parent.  Parentage is
> also under program control, so this is a programming error, too.
> 
> We have a bit over 500 callers.  Almost half of them pass
> &error_abort, slightly fewer ignore errors, one test case handles
> errors, and the remaining few callers pass them to their own callers.
> 
> The previous few commits demonstrated once again that ignoring
> programming errors is a bad idea.
> 
> Of the few ones that pass on errors, several violate the Error API.
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.  ich9_pm_add_properties(), sparc32_ledma_realize(),
> sparc32_dma_realize(), xilinx_axidma_realize(), xilinx_enet_realize()
> are wrong that way.
> 
> When the one appropriate choice of argument is &error_abort, letting
> users pick the argument is a bad idea.
> 
> Drop parameter @errp and assert the preconditions instead.
> 
> There's one exception to "duplicate property name is a programming
> error": the way object_property_add() implements the magic (and
> undocumented) "automatic arrayification".  Don't drop @errp there.
> Instead, rename object_property_add() to object_property_try_add(),
> and add the obvious wrapper object_property_add().

Huge. Could this last paragraph be done as a separate patch (ie. 
introduce object_property_try_add and adjust its clients), prior to the 
bulk mechanical patch that deletes the errp argument for all remaining 
instances?

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qom/object.h                |  81 +++-----

>   qom/container.c                     |   2 +-
>   qom/object.c                        | 302 +++++++++-------------------
>   qom/object_interfaces.c             |   5 +-

The core of the patch lies in these files, but even then it is still 
large because of adding a new API at the same time as fixing an existing 
one.

>   190 files changed, 643 insertions(+), 987 deletions(-)
> 

> +++ b/qom/object.c

> @@ -1129,12 +1123,12 @@ void object_unref(Object *obj)
>       }
>   }
>   
> -ObjectProperty *
> -object_property_add(Object *obj, const char *name, const char *type,
> -                    ObjectPropertyAccessor *get,
> -                    ObjectPropertyAccessor *set,
> -                    ObjectPropertyRelease *release,
> -                    void *opaque, Error **errp)
> +static ObjectProperty *
> +object_property_try_add(Object *obj, const char *name, const char *type,
> +                        ObjectPropertyAccessor *get,
> +                        ObjectPropertyAccessor *set,
> +                        ObjectPropertyRelease *release,
> +                        void *opaque, Error **errp)
>   {
>       ObjectProperty *prop;
>       size_t name_len = strlen(name);
> @@ -1148,8 +1142,8 @@ object_property_add(Object *obj, const char *name, const char *type,
>           for (i = 0; ; ++i) {
>               char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
>   
> -            ret = object_property_add(obj, full_name, type, get, set,
> -                                      release, opaque, NULL);
> +            ret = object_property_try_add(obj, full_name, type, get, set,
> +                                          release, opaque, NULL);
>               g_free(full_name);

Here's the magic in the last paragraph.

>               if (ret) {
>                   break;
> @@ -1179,6 +1173,17 @@ object_property_add(Object *obj, const char *name, const char *type,
>       return prop;
>   }
>   
> +ObjectProperty *
> +object_property_add(Object *obj, const char *name, const char *type,
> +                    ObjectPropertyAccessor *get,
> +                    ObjectPropertyAccessor *set,
> +                    ObjectPropertyRelease *release,
> +                    void *opaque)
> +{
> +    return object_property_try_add(obj, name, type, get, set, release,
> +                                   opaque, &error_abort);
> +}
> +

and if you were to split things into two patches, the first patch would 
be adding:

ObjectProperty *
object_property_add(Object *obj, const char *name, const char *type,
                     ObjectPropertyAccessor *get,
                     ObjectPropertyAccessor *set,
                     ObjectPropertyRelease *release,
                     void *opaque, Error **errp)
{
     return object_property_try_add(obj, name, type, get, set, release,
                                    opaque, errp);
}

with the second changing the signature to drop errp and forward 
&error_abort.


>   ObjectProperty *
>   object_class_property_add(ObjectClass *klass,
>                             const char *name,
> @@ -1186,16 +1191,11 @@ object_class_property_add(ObjectClass *klass,
>                             ObjectPropertyAccessor *get,
>                             ObjectPropertyAccessor *set,
>                             ObjectPropertyRelease *release,
> -                          void *opaque,
> -                          Error **errp)
> +                          void *opaque)
>   {
>       ObjectProperty *prop;
>   
> -    if (object_class_property_find(klass, name, NULL) != NULL) {
> -        error_setg(errp, "attempt to add duplicate property '%s' to class (type '%s')",
> -                   name, object_class_get_name(klass));
> -        return NULL;
> -    }
> +    assert(!object_class_property_find(klass, name, NULL));

If you do split, then deciding whether this type of cleanup belongs in 
the first patch (by ignoring the errp parameter, before mechanically 
dropping it) or the second is a fuzzier answer.

At any rate, my quick glance over the mechanical changes, and focused 
glance at these points of interest, sees nothing wrong.  So even if you 
do not split the patch, I can live with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org