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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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?

It could.  Patch would look like this:

   @@ -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);
                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, Error **errp)
   +{
   +    return object_property_try_add(obj, name, type, get, set, release,
   +                                   opaque, errp);
   +}
   +
    ObjectProperty *
    object_class_property_add(ObjectClass *klass,
                              const char *name,

Makes no sense on its own, so I'd have to explain in the commit message.
I doubt it's worthwhile.

>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   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.

It is large because we're fixing up 25 functions (object.h: 25
insertions(+), 56 deletions(-)), and their uses.  The new code is
actually quite small: 19 insertions(+), 8 deletions(-).

>>   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.

Hmm, that's another splitting idea: eliminate use of the @errp
parameters, then drop them.  I'll think about it.

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

Thanks!