[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 17/17] qom: Drop @errp parameter of object_property_del()
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 17/17] qom: Drop @errp parameter of object_property_del() |
Date: |
Sat, 02 May 2020 07:09:47 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Eric Blake <address@hidden> 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 <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()?
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.
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH 17/17] qom: Drop @errp parameter of object_property_del(),
Markus Armbruster <=