Same story as for object_property_add(): the only way
object_property_del() can fail is when the property with this name
does not exist.  Since our property names are all hardcoded, failure
is a programming error, and the appropriate way to handle it is
passing &error_abort.  Most callers do that, the commit before
previous fixed one that didn't (and got the error handling wrong), and
the two remaining exceptions ignore errors.

Drop the @errp parameter and assert the precondition instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qom/object.h       | 2 +-
 hw/core/qdev.c             | 2 +-
 hw/i386/pc_sysfw.c         | 2 +-
 hw/ppc/spapr_drc.c         | 4 ++--
 qom/object.c               | 7 +------
 qom/object_interfaces.c    | 3 +--
 tests/check-qom-proplist.c | 2 +-
 7 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 7458fefc7b..15af9dbbc8 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1047,7 +1047,7 @@ ObjectProperty *object_property_add(Object *obj, const char *name,
                                     ObjectPropertyRelease *release,
                                     void *opaque);
 
-void object_property_del(Object *obj, const char *name, Error **errp);
+void object_property_del(Object *obj, const char *name);
 
 ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name,
                                           const char *type,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 64ac9829bd..e26e5a75c9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -64,7 +64,7 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
             bus->num_children--;
 
             /* This gives back ownership of kid->child back to us.  */
-            object_property_del(OBJECT(bus), name, NULL);
+            object_property_del(OBJECT(bus), name);
             object_unref(OBJECT(kid->child));
             g_free(kid);
             return;
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 002133a2d8..2abab3a27c 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -120,7 +120,7 @@ static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
         dev_obj = OBJECT(pcms->flash[i]);
         if (!object_property_get_bool(dev_obj, "realized", &error_abort)) {
             prop_name = g_strdup_printf("pflash%d", i);
-            object_property_del(OBJECT(pcms), prop_name, &error_abort);
+            object_property_del(OBJECT(pcms), prop_name);
             g_free(prop_name);
             object_unparent(dev_obj);
             pcms->flash[i] = NULL;
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8b2171f698..b958f8acb5 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -405,7 +405,7 @@ static void spapr_drc_release(SpaprDrc *drc)
     g_free(drc->fdt);
     drc->fdt = NULL;
     drc->fdt_start_offset = 0;
-    object_property_del(OBJECT(drc), "device", &error_abort);
+    object_property_del(OBJECT(drc), "device");
     drc->dev = NULL;
 }
 
@@ -551,7 +551,7 @@ static void unrealize(DeviceState *d)
     vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
     name = g_strdup_printf("%x", spapr_drc_index(drc));
-    object_property_del(root_container, name, &error_abort);
+    object_property_del(root_container, name);
     g_free(name);
 }
 
diff --git a/qom/object.c b/qom/object.c
index 14c7efe127..424cc20987 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1280,15 +1280,10 @@ ObjectProperty *object_class_property_find(ObjectClass *klass, const char *name,
     return prop;
 }
 
-void object_property_del(Object *obj, const char *name, Error **errp)
+void object_property_del(Object *obj, const char *name)
 {
     ObjectProperty *prop = g_hash_table_lookup(obj->properties, name);
 
-    if (!prop) {
-        error_setg(errp, "Property '.%s' not found", name);
-        return;
-    }
-
     if (prop->release) {
         prop->release(obj, name, prop->opaque);
     }
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 54e14a3a14..5684ed0483 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -88,8 +88,7 @@ Object *user_creatable_add_type(const char *type, const char *id,
     user_creatable_complete(USER_CREATABLE(obj), &local_err);
     if (local_err) {
         if (id != NULL) {
-            object_property_del(object_get_objects_root(),
-                                id, &error_abort);
+            object_property_del(object_get_objects_root(), id);
         }
         goto out;
     }
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 84f48fe592..13a824cfae 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -280,7 +280,7 @@ static void dummy_bus_init(Object *obj)
 static void dummy_bus_unparent(Object *obj)
 {
     DummyBus *bus = DUMMY_BUS(obj);
-    object_property_del(obj->parent, "backend", NULL);
+    object_property_del(obj->parent, "backend");
     object_unparent(OBJECT(bus->backend));
 }
 
-- 
2.21.1

On 4/28/20 11:34 AM, Markus Armbruster wrote:
> Same story as for object_property_add(): the only way
> object_property_del() can fail is when the property with this name
> does not exist.  Since our property names are all hardcoded, failure
> is a programming error, and the appropriate way to handle it is
> passing &error_abort.  Most callers do that, the commit before
> previous fixed one that didn't (and got the error handling wrong), and
> the two remaining exceptions ignore errors.
> 
> Drop the @errp parameter and assert the precondition instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

I skipped review of 15/17 (it's less mechanical, and although the commit 
message was good, verifying that the patch matches the commit message is 
a bigger task).  But assuming it is right, then this one indeed makes sense.


> +++ b/qom/object.c
> @@ -1280,15 +1280,10 @@ ObjectProperty *object_class_property_find(ObjectClass *klass, const char *name,
>       return prop;
>   }
>   
> -void object_property_del(Object *obj, const char *name, Error **errp)
> +void object_property_del(Object *obj, const char *name)
>   {
>       ObjectProperty *prop = g_hash_table_lookup(obj->properties, name);
>   
> -    if (!prop) {
> -        error_setg(errp, "Property '.%s' not found", name);
> -        return;
> -    }
> -
>       if (prop->release) {
>           prop->release(obj, name, prop->opaque);
>       }

However, the commit message says you assert the precondition, whereas 
the code SEGVs rather than asserts if the precondition is not met.  In 
practice, both will flag the programmer error, so I don't care which you 
do, but it's worth making the commit match the intent: Did you mean to 
add an assert()?


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

Eric Blake <eblake@redhat.com> writes:

> On 4/28/20 11:34 AM, Markus Armbruster wrote:
>> Same story as for object_property_add(): the only way
>> object_property_del() can fail is when the property with this name
>> does not exist.  Since our property names are all hardcoded, failure
>> is a programming error, and the appropriate way to handle it is
>> passing &error_abort.  Most callers do that, the commit before
>> previous fixed one that didn't (and got the error handling wrong), and
>> the two remaining exceptions ignore errors.
>>
>> Drop the @errp parameter and assert the precondition instead.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> I skipped review of 15/17 (it's less mechanical, and although the
> commit message was good, verifying that the patch matches the commit
> message is a bigger task).  But assuming it is right, then this one
> indeed makes sense.
>
>
>> +++ b/qom/object.c
>> @@ -1280,15 +1280,10 @@ ObjectProperty *object_class_property_find(ObjectClass *klass, const char *name,
>>       return prop;
>>   }
>>   -void object_property_del(Object *obj, const char *name, Error
>> **errp)
>> +void object_property_del(Object *obj, const char *name)
>>   {
>>       ObjectProperty *prop = g_hash_table_lookup(obj->properties, name);
>>   -    if (!prop) {
>> -        error_setg(errp, "Property '.%s' not found", name);
>> -        return;
>> -    }
>> -
>>       if (prop->release) {
>>           prop->release(obj, name, prop->opaque);
>>       }
>
> However, the commit message says you assert the precondition, whereas
> the code SEGVs rather than asserts if the precondition is not met.  In
> practice, both will flag the programmer error, so I don't care which
> you do, but it's worth making the commit match the intent: Did you
> mean to add an assert()?

I started with an assert, then decided asserting prop right before
dereferncing it is silly, and deleted the assertion without adjusting
the commit message.  I'll tidy up.