Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL

[Peter Crosthwaite]

On Fri, Nov 29, 2013 at 5:56 PM, Markus Armbruster <address@hidden> wrote:
> Peter Crosthwaite <address@hidden> writes:
>
>> 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().
>
> To make further progress, we need a patch.  Care to cook one up?
>

Done.

Regards,
Peter