From: Markus Armbruster <armbru@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2 0/3] error: allow local errors to trigger abort
Date: Thu, 18 Jun 2015 18:34:15 +0200 [thread overview]
Message-ID: <87a8vxrn5k.fsf@blackfin.pond.sub.org> (raw)
In-Reply-To: <1434525861-21768-1-git-send-email-mst@redhat.com> (Michael S. Tsirkin's message of "Wed, 17 Jun 2015 09:24:40 +0200")
"Michael S. Tsirkin" <mst@redhat.com> writes:
> It's a common idiom:
>
> Error *local_err = NULL;
> ....
> foo(&local_err);
> ...
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> Unfortunately it means that call to foo(&local_err) will
> not abort even if errp is set to error_abort.
>
> Instead, we get an abort at error_propagate which is too late,
> that is, the quality of the stack trace is degraded in that it no longer
> pinpoints the actual cause of failure.
>
> To fix, add an API to check errp and set local_err to error_abort
> if errp is error_abort.
>
> This is out of RFC but I'm still not converting all users:
> let's merge these patches, then I'll convert all users
> on top.
Let's take a step back and review intended use of Error before and after
this series.
Before:
* Parameter Error **errp (by convention the last one)
To create and return an error, do:
error_setg(errp, ...);
To propagate an existing error (typically one you received from a
callee), do:
error_propagate(errp, ...);
You're not supposed to examine errp, let alone dereference it.
* Actual argument
- to receive an error: address of an Error * variable, its value must
be null before the call, and may be examined afterwards
- to ignore errors: null pointer
- to abort on error: &error_abort
This leads to a few patterns:
T foo(..., Error **errp)
{
// pattern: receive, test and propagate error
Error *err = NULL;
bar(..., &err);
if (err) {
error_propagate(errp, err);
return ...;
}
// pattern: set error
if (...) {
error_setg(errp, ...);
return ...;
}
// pattern: pass through error
// really the first pattern less the test simplified
baz(..., errp);
return ...;
}
Your patch modifies the "actual argument to receive an error" clause:
the variable must now be either null or the value of
error_init_local(errp). If it's the latter, then it must be passed to
error_propagate(errp, err).
We acquire a new pattern:
// pattern: receive, test and propagate error
Error *err = error_init_local(errp);
bar(..., &err);
if (err) {
error_propagate(errp, err);
return ...;
}
Let's see whether it can fully replace the existing pattern. Before you
can convert err = NULL to err = error_init_local(errp), you have to:
* find the appropriate errp (usually trivial)
* double-check we error_propagate(errp, err) on every path leaving the
function
It's perfectly possible that we *don't* propagate on all paths:
Error *err = NULL;
bar(..., &err);
if (error is non-recoverable) {
error_propagate(errp, err);
return ...;
}
recover and carry on
Here, error_init_local(errp) would be *wrong*.
The Error boilerplate is annoying, but at least there are few ways to
get it wrong (the most common one is "Error * variable not null before
the call"). I'm most reluctant to add more ways to get it wrong.
Is the gain worth all this additional complexity? The gain is certainly
real: backtrace shows where the error got created instead of where it
was propagated to &error_abort. But the existing backtraces have never
bothered me. When I get one ending at an error_propagate(errp, err),
and I need the real one instead, finding it in the debugger is easy
enough. The complexity does bother me.
Here's an utterly trivial way to get some of the gain for none of the
complexity: make error_setg() & friends store caller's __FILE__ and
__LINE__.
next prev parent reply other threads:[~2015-06-18 16:34 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-17 7:24 [Qemu-devel] [PATCH v2 0/3] error: allow local errors to trigger abort Michael S. Tsirkin
2015-06-17 7:24 ` [Qemu-devel] [PATCH v2 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
2015-06-17 15:21 ` Eric Blake
2015-06-17 15:41 ` Eric Blake
2015-06-18 15:36 ` Markus Armbruster
2015-06-18 16:10 ` Markus Armbruster
2015-06-17 7:24 ` [Qemu-devel] [PATCH v2 2/3] error: allow local errors to trigger abort Michael S. Tsirkin
2015-06-17 7:24 ` [Qemu-devel] [PATCH v2 3/3] block/nfs: switch to error_init_local Michael S. Tsirkin
2015-06-17 15:32 ` Eric Blake
2015-06-23 9:03 ` Michael S. Tsirkin
2015-06-18 16:34 ` Markus Armbruster [this message]
2015-06-18 16:49 ` [Qemu-devel] [PATCH v2 0/3] error: allow local errors to trigger abort Paolo Bonzini
2015-06-22 11:31 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a8vxrn5k.fsf@blackfin.pond.sub.org \
--to=armbru@redhat.com \
--cc=dgilbert@redhat.com \
--cc=kwolf@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.