On 10/2/19 5:15 AM, Roman Kagan wrote:
On Tue, Oct 01, 2019 at 06:52:52PM +0300, Vladimir Sementsov-Ogievskiy wrote:
Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.
It has three goals:
1. Fix issue with error_fatal & error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+#define ERRP_AUTO_PROPAGATE() \
+g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
+errp = ((errp == NULL || *errp == error_fatal) ? \
+ &__auto_errp_prop.local_err : errp)
+
I guess it has been discussed but I couldn't find it, so: what's the
reason for casting in stone the name of the function parameter, which
isn't quite so obvious when you see this macro used in the code? IMO
if the macro took errp as a parameter that would be easier to follow.
It was discussed. Forcing a specific name of the 'Error **errp' has
tradeoffs:
Pro: possible to write a sed script to check for missing spots in the
macros (in fact, my sed script found spots missed by Coccinelle when not
using the correct --macro-header option)
Pro: enforces uniform style
Con: misses instances that have typos or otherwise used the wrong name
Allowing the macro to take the name of the parameter:
Pro/Con: More flexible (allows use in more place, but loses consistency)
Pro: Coccinelle can still handle the conversion
Con: using sed to check if Coccinelle missed anything is harder
But opinions on how to paint the bikeshed are still welcome; it's easy
enough to respin the series with the macro taking an argument if that
is agreed to indeed be more legible.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
