Re: [Qemu-ppc] [RFC] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

19.09.2019 18:24, Eric Blake wrote:
> 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...).

When I was a new contributor, it was not simple to understand, when and why we
need to use local_err, keeping in mind that (this is not only reason, but also
a consequence) we have places where local_err is used for no reason.

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


-- 
Best regards,
Vladimir