Eric Blake <ebl...@redhat.com> 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 <arm...@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.
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 <ebl...@redhat.com>
Thanks!
