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

[Daniel P . Berrangé]

On Thu, Sep 19, 2019 at 10:21:44AM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 13:09, Daniel P. Berrangé wrote:
> > 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.
> > 
> > 
> 
> Agree. But seems that such change may be only done by hand.. Hmm, on the 
> other hand,
> why not. May be I'll try do it for some parts of block layer.
> 
> Still, error_append_hint needs local_err in case of error_fatal.

Yep, fortunately that usage is comparatively rare vs use of error_propagate
in general.

To be clear I don't have any objections to your overall idea of using
attribute cleanup to simplify error propagation. As you say, it can
still be useful for the error_append_hint, even if we use the code
pattern I suggest more widely to eliminate propagation where possible.

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 :|