Re: [qemu-s390x] [RFC] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

19.09.2019 13:09, Daniel P. Berrangé wrote:
> On Thu, Sep 19, 2019 at 11:17:20AM +0200, Kevin Wolf wrote:
>> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> + */
>>>> +#define MAKE_ERRP_SAFE(errp) \
>>>> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
>>>> +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) { 
>>>> \
>>>> +    (errp) = &__auto_errp_prop.local_err; \
>>>> +}
>>>
>>> Not written to take a trailing semicolon in the caller.
>>>
>>> You could even set __auto_errp_prop unconditionally rather than trying
>>> to reuse incoming errp (the difference being that error_propagate() gets
>>> called more frequently).
>>
>> I think this difference is actually a problem.
>>
>> When debugging things, I hate error_propagate(). It means that the Error
>> (specifically its fields src/func/line) points to the outermost
>> error_propagate() rather than the place where the error really happened.
>> It also makes error_abort completely useless because at the point where
>> the process gets aborted, the interesting information is already lost.
> 
> Yeah, that is very frustrating. For personal development you can eventually
> figure it out, but with customer support requests, all you get is the
> stack trace and you've no core file to investigate, so you're stuck.
> IOW, as a general principle, we should strive to minimize the usage
> of error propagate.
> 
> The key reason why we typically need the propagate calls, is because
> we allow the passed in Error **errp parameter to be NULL, while at the
> same time we want to inspect it to see if its contents are non-NULL
> to detect failure. I venture to suggest that this is flawed API
> design on our part. We should not need to inspect the error object
> to see if a method call fails - the return value can be used for
> this purpose.
> 
> IOW, instead of this pattern with typically 'void' methods
> 
>       extern void somemethod(Error **errp);
> 
>       void thismethod(Error **errp) {
>          Error *local_err = NULL;
>       ...
>          somemethod(&local_err);
>          if (local_err) {
>           error_propagate(errp, local_err);
>           return;
>       }
>          ...
>       }
> 
> We should be preferring
> 
>       extern int somemethod(Error **errp);
> 
>       int thismethod(Error **errp) {
>       ...
>          if (somemethod(errp) < 0) {
>           return -1;
>       }
>          ...
>       }
> 
> ie only using the Error object to bubble up the *details* of
> the error, not as the mechanism for finding if an error occurred.
> 
> There is of course a downside with this approach, in that a
> method might set 'errp' but return 0, or not set 'errp' but
> return -1. I think this is outweighed by the simpler code
> pattern overall though.
> 
> 

Agree. But seems that such change may be only done by hand.. Hmm, on the other 
hand,
why not. May be I'll try do it for some parts of block layer.

Still, error_append_hint needs local_err in case of error_fatal.


-- 
Best regards,
Vladimir