Re: [PATCH 17/17] qom: Drop @errp parameter of object_property_del()

[Eric Blake]

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

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