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