Re: [RFC] error: auto propagated local_err

[Eric Blake]

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

>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
>> **errp parameter is dirt-simple to explain.  It has no performance
>> penalty if the user passed in a normal error or error_abort (the cost of
>> an 'if' hidden in the macro is probably negligible compared to
>> everything else we do), and has no semantic penalty if the user passed
>> in NULL or error_fatal (we now get the behavior we want with less
>> boilerplate).
>>
>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
>> can I omit it?' does not provide the same simplicity.
> 
> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
> really know what its doing without looking at it, and this is QEMU
> custom concept so one more thing to learn for new contributors.
> 
> While I think it is a nice trick, personally I think we would be better
> off if we simply used a code pattern which does not require de-referencing
> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
> of all our methods using Error **errp.

If 100% of our callsites use the macro, then new contributors will
quickly learn by observation alone that the macro usage must be
important on any new function taking Error **errp, whether or not they
actually read the macro to see what it does.  If only 5% of our
callsites use the macro, it's harder to argue that a new user will pick
up on the nuances by observation alone (presumably, our docs would also
spell it out, but we know that not everyone reads those...).

However, if we can automate syntax checks to reach a near-100% accuracy,
we don't HAVE to worry about whether a new programmer picks up on the
nuances by observation, because they will instead pick up the nuances by
CI rejection messages.  This is true for _either_ style:

100% use of the macro: CI message would be "you added a method with a
parameter 'Error **errp' but forgot to use MAKE_ERRP_SAFE"

use of the macro only where necessary (namely, functions that contain
'*errp' and/or 'error_append_hint'): CI message would either be "your
function body requires MAKE_ERRP_SAFE but you forgot it" or "your
function body does not require MAKE_ERRP_SAFE but you forgot to remove
it".  And this would work even for experienced committers editing
existing functions (such as ongoing work to convert away from 'void
child(errp); if (*errp)' and towards 'if (int child(errp) < 0)').

Writing the CI engine for the first case is easy, writing it for the
second is a bit harder, but still seems tractable (since, for any
function with an 'Error **errp' parameter, it should be easy to scan the
function body for instances of '*errp' or 'error_append_hint', as well
as to scan whether MAKE_ERRP_SAFE was present or absent accordingly).

> 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.

I'd really like to hear Markus' opinion on the matter, as Error maintainer.

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