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