qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [RFC v5 024/126] error: auto propagated local_err
Date: Thu, 5 Dec 2019 11:32:41 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]