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

[Markus Armbruster <armbru@redhat.com>]

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.