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

[Markus Armbruster]

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?