qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if calle


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL
Date: Fri, 29 Nov 2013 10:21:42 +1000

On Fri, Nov 29, 2013 at 1:03 AM, Markus Armbruster <address@hidden> wrote:
> Paolo Bonzini <address@hidden> writes:
>
>> Il 28/11/2013 14:23, Igor Mammedov ha scritto:
>>> > object_property_set(Foo, bar, "baz", &abort_on_err);
>>>
>>> that is just another way to put burden on caller, instead of doing it
>>> in one place.
>>
>> It's also much more self-documenting.
>>
>> The problem is that there is one very good case where you want the
>> silent-don't-care behavior: when you don't care about the exact error,
>> and you can figure out whether there was an error from the returned
>> value of the function.  This doesn't apply to object_property_set of
>> course, but it is the reason why NULL Error* has silent-don't-care behavior.
>>
>> Now, let's look at the alternatives:
>>
>> * keep silent don't care
>>   + consistent
>>   + predictable
>>   - not always handy
>>
>> * only modify object_property_set
>>   + mostly does the right thing
>>   - inconsistent with other Error* functions
>>   - inconsistent with _nofail functions
>>
>> * Peter's alternative
>>   + self-documenting
>>   + consistent
>>   + predictable
>>
>> * make Error* mandatory for all void functions
>>   + consistent
>>   + almost predictable (because in C you can ignore return values)
>>   - not necessarily does the right thing (e.g. cleanup functions)
>>   - requires manual effort to abide to the policy
>>
>> I vote for Peter's proposal, or for adding object_property_set_nofail.
>> No particular preference.
>>
>> Another variant: modify object_property_set to add a g_warning.  I think
>> it's fine.  It reduces the inconsistency, and still simplifies debugging.
>
> I like Peter's proposal, provided we use it to get rid of the _nofail
> pattern.
>
> Second preference is adding another _nofail wrapper.
>

So this issue with _nofail is that if you are doing it properly, every
API needed both normal and _nofail version of every function. APIs
generally have no bussiness dictating failure policy so by extension,
normal and _nofail should exist for every API that accepts an Error *.
With my proposal, its fixed once, in one place and we can torch all
the _nofail boilerplate all over the tree as you have suggested.

These is another subtle advantage to my proposal, and that is that
assertions can happen at the point of failure in the offending API,
not after the fact in the caller. If the caller does an
assert_no_error, then the abort() happens after return from the API
call. This makes debugging awkward cause the stack frames into the API
call where the issue actually occured are lost. Whereas if error_set
does the abort() you will get all stack frames into the API call where
the issue occured when gdb traps the abort().

Regards,
Peter



reply via email to

[Prev in Thread] Current Thread [Next in Thread]