Eric Blake <ebl...@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 <arm...@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.
