Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err

[Eric Blake <eblake@xxxxxxxxxx>]

On 12/5/19 8:58 AM, Vladimir Sementsov-Ogievskiy wrote:

> > > > What about you provide the examples, and then I try to polish the prose?
> > > 1: error_fatal problem
> > >
> > > Assume the following code flow:
> > >
> > > int f1(errp) {
> > >       ...
> > >       ret = f2(errp);
> > >       if (ret < 0) {
> > >          error_append_hint(errp, "very useful hint");
> > >          return ret;
> > >       }
> > >       ...
> > > }
> > >
> > > Now, if we call f1 with &error_fatal argument and f2 fails, the program
> > > will exit immediately inside f2, when setting the errp. User will not
> > > see the hint.
> > >
> > > So, in this case we should use local_err.
> > How does this example look after the transformation?
Without ERRP_AUTO_PROPAGATE(), the transformation is a lot of boilerplate:

int f1(errp) {
    Error *err = NULL;
    ret = f2(&err);
    if (ret < 0) {
        error_append_hint(&err, "very useful hint");
        error_propagate(errp, err);
        return ret;
    }
}

what's worse, that boilerplate to solve problem 1 turns out to be...

> Good point.
>
> int f1(errp) {
>      ERRP_AUTO_PROPAGATE();
>      ...
>      ret = f2(errp);
>      if (ret < 0) {
>         error_append_hint(errp, "very useful hint");
>         return ret;
>      }
>      ...
> }
>
> - nothing changed, only add macro at start. But now errp is safe, if it was
> error_fatal it is wrapped by local error, and will only call exit on automatic
> propagation on f1 finish.
>
> > > 2: error_abort problem
> > >
> > > Now, consider functions without return value. We normally use local_err
> > > variable to catch failures:
> > >
> > > void f1(errp) {
> > >       Error *local_err = NULL;
> > >       ...
> > >       f2(&local_err);
> > >       if (local_err) {
> > >           error_propagate(errp, local_err);
> > >           return;
> > >       }
> > >       ...
> > > }
the very same code as the cause of problem 2.

> > > Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
> > > crash dump will point to error_propagate, not to the failure point in f2,
> > > which complicates debugging.
> > >
> > > So, we should never wrap error_abort by local_err.
> > Likewise.
> And here:
>
> void f1(errp) {
>       ERRP_AUTO_PROPAGATE();
>       ...
>       f2(errp);
>       if (*errp) {
>           return;
>       }
>       ...
>
> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
>     local error is automatically propagated to original one.
So, the use of ERRP_AUTO_PROPAGATE() solves BOTH problems 1 and 2 - we 
avoid the boilerplate that trades one problem for another, by 
consolidating ALL of the boilerplate into a single-line macro, such that 
error_propagate() no longer needs to be called anywhere except inside 
the ERRP_AUTO_PROPAGATE macro.
> > > ===
> > >
> > > Our solution:
> > >
> > > - Fixes [1.], adding invocation of new macro into functions with 
> > > error_appen_hint/error_prepend,
> > >      New macro will wrap error_fatal.
> > > - Fixes [2.], by switching from hand-written local_err to smart macro, which 
> > > never
> > >      wraps error_abort.
> > > - Handles [3.], by switching to macro, which is less code
> > > - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra 
> > > propagations
> > >      (in fact, error_propagate is called, but returns immediately on first if 
> > > (!local_err))
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel