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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Sep 19, 2019 at 11:17:20AM +0200, Kevin Wolf wrote:
> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
> > On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > + */
> > > +#define MAKE_ERRP_SAFE(errp) \
> > > +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
> > > +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) { \
> > > +    (errp) = &__auto_errp_prop.local_err; \
> > > +}
> > 
> > Not written to take a trailing semicolon in the caller.
> > 
> > You could even set __auto_errp_prop unconditionally rather than trying
> > to reuse incoming errp (the difference being that error_propagate() gets
> > called more frequently).
> 
> I think this difference is actually a problem.
> 
> When debugging things, I hate error_propagate(). It means that the Error
> (specifically its fields src/func/line) points to the outermost
> error_propagate() rather than the place where the error really happened.
> It also makes error_abort completely useless because at the point where
> the process gets aborted, the interesting information is already lost.

Yeah, that is very frustrating. For personal development you can eventually
figure it out, but with customer support requests, all you get is the
stack trace and you've no core file to investigate, so you're stuck.
IOW, as a general principle, we should strive to minimize the usage
of error propagate.

The key reason why we typically need the propagate calls, is because
we allow the passed in Error **errp parameter to be NULL, while at the
same time we want to inspect it to see if its contents are non-NULL
to detect failure. I venture to suggest that this is flawed API
design on our part. We should not need to inspect the error object
to see if a method call fails - the return value can be used for
this purpose.

IOW, instead of this pattern with typically 'void' methods

     extern void somemethod(Error **errp);

     void thismethod(Error **errp) {
        Error *local_err = NULL;
	...
        somemethod(&local_err);
        if (local_err) {
	    error_propagate(errp, local_err);
	    return;
	}
        ...
     }

We should be preferring

     extern int somemethod(Error **errp);

     int thismethod(Error **errp) {
	...
        if (somemethod(errp) < 0) {
	    return -1;
	}
        ...
     }

ie only using the Error object to bubble up the *details* of
the error, not as the mechanism for finding if an error occurred.

There is of course a downside with this approach, in that a
method might set 'errp' but return 0, or not set 'errp' but
return -1. I think this is outweighed by the simpler code
pattern overall though.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|