Re: [RFC] error: auto propagated local_err

[Eric Blake <eblake@redhat.com>]

On 9/19/19 10:50 AM, Daniel P. Berrangé wrote:

> To get some slightly less made-up stats, I did some searching:
> 
>   - 4408  methods with an 'errp' parameter declared
> 
>      git grep 'Error \*\*errp'|  wc -l
> 
>   - 696 methods with an 'Error *local' declared (what other names
>     do we use for this?)
> 
>      git grep 'Error \*local' | wc -l

Covers 'local' and 'local_err'.  We also have:

git grep 'Error \*err\b' | wc -l
553

So 1249 local errors.

> 
>   - 1243 methods with an 'errp' parameter which have void
>     return value (fuzzy number - my matching is quite crude)
> 
>      git grep 'Error \*\*errp'|  grep -E '(^| )void' | wc -l
> 
>   - 11 methods using error_append_hint with a local_err
> 
>      git grep append_hint | grep local | wc -l
> 
> 
> This suggests to me, that if we used the 'return 0 / return -1' convention
> to indicate failure for the methods which currently return void, we could
> potentially only have 11 cases where a local error object is genuinely
> needed. 
> 
> If my numbers are at all accurate, I still believe we're better off
> changing the return type and eliminating essentially all use of local
> error variables, as void funcs/local error objects appear to be the
> minority coding pattern.

When relying on a return value, you do have to check whether a negative
return value happens if and only if errp is set; that's something that's
harder for code grepping to spot.  I'm not opposed to that coding
pattern, just pointing out that it requires some semantic analysis,
while MAKE_ERRP_SAFE() coupled with checking *errp is easier to prove at
a glance that the check for whether an error happened is 1:1 associated
with whether an error is reported.

> 
> 
>>> There are lots of neat things we could do with auto-cleanup functions we
>>> I think we need to be wary of hiding too much cleverness behind macros
>>> when doing so overall.
>>
>> The benefit of getting rid of the 'Error *local_err = NULL; ...
>> error_propagate()' boilerplate is worth the cleverness, in my opinion,
>> but especially if also accompanied by CI coverage that we abide by our
>> new rules.
> 
> At least we're both wanting to eliminate the local error + propagation.
> The difference is whether we're genuinely eliminating it, or just hiding
> it from view via auto-cleanup & macro magic
> 
> Regards,
> Daniel
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org