Re: [PATCH v4 04/31] error: auto propagated local_err

[Eric Blake]

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