* [PATCH v12 0/8] error: auto propagated local_err part I
@ 2020-07-07 16:50 Markus Armbruster
2020-07-07 16:50 ` Markus Armbruster
` (9 more replies)
0 siblings, 10 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
To speed things up, I'm taking the liberty to respin Vladimir's series
with my documentation amendments.
After my documentation work, I'm very much inclined to rename
ERRP_AUTO_PROPAGATE() to ERRP_GUARD(). The fact that it propagates
below the hood is detail. What matters to its users is that it lets
them use @errp more freely. Thoughts?
Based-on: Message-Id: <20200707160613.848843-1-armbru@redhat.com>
Available from my public repository https://repo.or.cz/qemu/armbru.git
on branch error-auto.
v12: (based on "[PATCH v4 00/45] Less clumsy error checking")
01: Comments merged properly with recent commit "error: Document Error
API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
before its helpers, and touch up style.
01-08: Commit messages tweaked
Vladimir's cover letter for v11:
v11: (based-on "[PATCH v2 00/44] Less clumsy error checking")
01: minor rebase of documentation, keep r-bs
02: - minor comment tweaks [Markus]
- use explicit file name in MAINTAINERS instead of pattern
- add Markus's r-b
03,07,08: rabase changes, drop r-bs
v11 is available at
https://src.openvz.org/scm/~vsementsov/qemu.git #tag up-auto-local-err-partI-v11
v10 is available at
https://src.openvz.org/scm/~vsementsov/qemu.git #tag up-auto-local-err-partI-v10
In these series, there is no commit-per-subsystem script, each generated
commit is generated in separate.
Still, generating commands are very similar, and looks like
sed -n '/^<Subsystem name>$/,/^$/{s/^F: //p}' MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Note, that in each generated commit, generation command is the only
text, indented by 8 spaces in 'git log -1' output, so, to regenerate all
commits (for example, after rebase, or change in coccinelle script), you
may use the following command:
git rebase -x "sh -c \"git show --pretty= --name-only | xargs git checkout HEAD^ -- ; git reset; git log -1 | grep '^ ' | sh\"" HEAD~6
Which will start automated interactive rebase for generated patches,
which will stop if generated patch changed
(you may do git commit --amend to apply updated generated changes).
Note:
git show --pretty= --name-only - lists files, changed in HEAD
git log -1 | grep '^ ' | sh - rerun generation command of HEAD
Check for compilation of changed .c files
git rebase -x "sh -c \"git show --pretty= --name-only | sed -n 's/\.c$/.o/p' | xargs make -j9\"" HEAD~6
Vladimir Sementsov-Ogievskiy (8):
error: New macro ERRP_AUTO_PROPAGATE()
scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
sd: Use ERRP_AUTO_PROPAGATE()
pflash: Use ERRP_AUTO_PROPAGATE()
fw_cfg: Use ERRP_AUTO_PROPAGATE()
virtio-9p: Use ERRP_AUTO_PROPAGATE()
nbd: Use ERRP_AUTO_PROPAGATE()
xen: Use ERRP_AUTO_PROPAGATE()
scripts/coccinelle/auto-propagated-errp.cocci | 337 ++++++++++++++++++
include/block/nbd.h | 1 +
include/qapi/error.h | 163 ++++++++-
block/nbd.c | 7 +-
hw/9pfs/9p-local.c | 12 +-
hw/9pfs/9p.c | 1 +
hw/block/dataplane/xen-block.c | 17 +-
hw/block/pflash_cfi01.c | 7 +-
hw/block/pflash_cfi02.c | 7 +-
hw/block/xen-block.c | 102 +++---
hw/nvram/fw_cfg.c | 14 +-
hw/pci-host/xen_igd_pt.c | 7 +-
hw/sd/sdhci-pci.c | 7 +-
hw/sd/sdhci.c | 21 +-
hw/sd/ssi-sd.c | 10 +-
hw/xen/xen-backend.c | 7 +-
hw/xen/xen-bus.c | 92 ++---
hw/xen/xen-host-pci-device.c | 27 +-
hw/xen/xen_pt.c | 25 +-
hw/xen/xen_pt_config_init.c | 17 +-
nbd/client.c | 5 +
nbd/server.c | 5 +
MAINTAINERS | 1 +
23 files changed, 659 insertions(+), 233 deletions(-)
create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
--
2.26.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE() Markus Armbruster
` (8 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.
It has three goals:
1. Fix issue with error_fatal and error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]
2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]
3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).
To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
[Comments merged properly with recent commit "error: Document Error
API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
before its helpers, and touch up style. Commit message tweaked.]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/error.h | 160 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 141 insertions(+), 19 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3fed49747d..c865a7d2f1 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -30,6 +30,10 @@
* job. Since the value of @errp is about handling the error, the
* function should not examine it.
*
+ * - The function may pass @errp to functions it calls to pass on
+ * their errors to its caller. If it dereferences @errp to check
+ * for errors, it must use ERRP_AUTO_PROPAGATE().
+ *
* - On success, the function should not touch *errp. On failure, it
* should set a new error, e.g. with error_setg(errp, ...), or
* propagate an existing one, e.g. with error_propagate(errp, ...).
@@ -45,15 +49,17 @@
* = Creating errors =
*
* Create an error:
- * error_setg(&err, "situation normal, all fouled up");
+ * error_setg(errp, "situation normal, all fouled up");
+ * where @errp points to the location to receive the error.
*
* Create an error and add additional explanation:
- * error_setg(&err, "invalid quark");
- * error_append_hint(&err, "Valid quarks are up, down, strange, "
+ * error_setg(errp, "invalid quark");
+ * error_append_hint(errp, "Valid quarks are up, down, strange, "
* "charm, top, bottom.\n");
+ * This may require use of ERRP_AUTO_PROPAGATE(); more on that below.
*
* Do *not* contract this to
- * error_setg(&err, "invalid quark\n" // WRONG!
+ * error_setg(errp, "invalid quark\n" // WRONG!
* "Valid quarks are up, down, strange, charm, top, bottom.");
*
* = Reporting and destroying errors =
@@ -107,18 +113,6 @@
* Errors get passed to the caller through the conventional @errp
* parameter.
*
- * Pass an existing error to the caller:
- * error_propagate(errp, err);
- * where Error **errp is a parameter, by convention the last one.
- *
- * Pass an existing error to the caller with the message modified:
- * error_propagate_prepend(errp, err,
- * "Could not frobnicate '%s': ", name);
- * This is more concise than
- * error_propagate(errp, err); // don't do this
- * error_prepend(errp, "Could not frobnicate '%s': ", name);
- * and works even when @errp is &error_fatal.
- *
* Create a new error and pass it to the caller:
* error_setg(errp, "situation normal, all fouled up");
*
@@ -128,18 +122,26 @@
* handle the error...
* }
* when it doesn't, say a void function:
+ * ERRP_AUTO_PROPAGATE();
+ * foo(arg, errp);
+ * if (*errp) {
+ * handle the error...
+ * }
+ * More on ERRP_AUTO_PROPAGATE() below.
+ *
+ * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
- * error_propagate(errp, err);
+ * error_propagate(errp, err); // deprecated
* }
- * Do *not* "optimize" this to
+ * Avoid in new code. Do *not* "optimize" it to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
- * because errp may be NULL!
+ * because errp may be NULL! Guard with ERRP_AUTO_PROPAGATE().
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
@@ -158,6 +160,19 @@
* handle the error...
* }
*
+ * Pass an existing error to the caller:
+ * error_propagate(errp, err);
+ * This is rarely needed. When @err is a local variable, use of
+ * ERRP_AUTO_PROPAGATE() commonly results in more readable code.
+ *
+ * Pass an existing error to the caller with the message modified:
+ * error_propagate_prepend(errp, err,
+ * "Could not frobnicate '%s': ", name);
+ * This is more concise than
+ * error_propagate(errp, err); // don't do this
+ * error_prepend(errp, "Could not frobnicate '%s': ", name);
+ * and works even when @errp is &error_fatal.
+ *
* Receive and accumulate multiple errors (first one wins):
* Error *err = NULL, *local_err = NULL;
* foo(arg, &err);
@@ -185,6 +200,70 @@
* error_setg(&err, ...); // WRONG!
* }
* because this may pass a non-null err to error_setg().
+ *
+ * = Why, when and how to use ERRP_AUTO_PROPAGATE() =
+ *
+ * Without ERRP_AUTO_PROPAGATE(), use of the @errp parameter is
+ * restricted:
+ * - It must not be dereferenced, because it may be null.
+ * - It should not be passed to error_prepend() or
+ * error_append_hint(), because that doesn't work with &error_fatal.
+ * ERRP_AUTO_PROPAGATE() lifts these restrictions.
+ *
+ * To use ERRP_AUTO_PROPAGATE(), add it right at the beginning of the
+ * function. @errp can then be used without worrying about the
+ * argument being NULL or &error_fatal.
+ *
+ * Using it when it's not needed is safe, but please avoid cluttering
+ * the source with useless code.
+ *
+ * = Converting to ERRP_AUTO_PROPAGATE() =
+ *
+ * To convert a function to use ERRP_AUTO_PROPAGATE():
+ *
+ * 0. If the Error ** parameter is not named @errp, rename it to
+ * @errp.
+ *
+ * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
+ * the beginning of the function. This makes @errp safe to use.
+ *
+ * 2. Replace &err by errp, and err by *errp. Delete local variable
+ * @err.
+ *
+ * 3. Delete error_propagate(errp, *errp), replace
+ * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
+ *
+ * 4. Ensure @errp is valid at return: when you destroy *errp, set
+ * errp = NULL.
+ *
+ * Example:
+ *
+ * bool fn(..., Error **errp)
+ * {
+ * Error *err = NULL;
+ *
+ * foo(arg, &err);
+ * if (err) {
+ * handle the error...
+ * error_propagate(errp, err);
+ * return false;
+ * }
+ * ...
+ * }
+ *
+ * becomes
+ *
+ * bool fn(..., Error **errp)
+ * {
+ * ERRP_AUTO_PROPAGATE();
+ *
+ * foo(arg, errp);
+ * if (*errp) {
+ * handle the error...
+ * return false;
+ * }
+ * ...
+ * }
*/
#ifndef ERROR_H
@@ -285,6 +364,7 @@ void error_setg_win32_internal(Error **errp,
* the error object.
* Else, move the error object from @local_err to *@dst_errp.
* On return, @local_err is invalid.
+ * Please use ERRP_AUTO_PROPAGATE() instead when possible.
* Please don't error_propagate(&error_fatal, ...), use
* error_report_err() and exit(), because that's more obvious.
*/
@@ -296,6 +376,8 @@ void error_propagate(Error **dst_errp, Error *local_err);
* Behaves like
* error_prepend(&local_err, fmt, ...);
* error_propagate(dst_errp, local_err);
+ * Please use ERRP_AUTO_PROPAGATE() and error_prepend() instead when
+ * possible.
*/
void error_propagate_prepend(Error **dst_errp, Error *local_err,
const char *fmt, ...);
@@ -393,6 +475,46 @@ void error_set_internal(Error **errp,
ErrorClass err_class, const char *fmt, ...)
GCC_FMT_ATTR(6, 7);
+/*
+ * Make @errp parameter easier to use regardless of argument value
+ *
+ * This macro is for use right at the beginning of a function that
+ * takes an Error **errp parameter to pass errors to its caller. The
+ * parameter must be named @errp.
+ *
+ * It must be used when the function dereferences @errp or passes
+ * @errp to error_prepend(), error_vprepend(), or error_append_hint().
+ * It is safe to use even when it's not needed, but please avoid
+ * cluttering the source with useless code.
+ *
+ * If @errp is NULL or &error_fatal, rewrite it to point to a local
+ * Error variable, which will be automatically propagated to the
+ * original @errp on function exit.
+ *
+ * Note: &error_abort is not rewritten, because that would move the
+ * abort from the place where the error is created to the place where
+ * it's propagated.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+ g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+ do { \
+ if (!errp || errp == &error_fatal) { \
+ errp = &_auto_errp_prop.local_err; \
+ } \
+ } while (0)
+
+typedef struct ErrorPropagator {
+ Error *local_err;
+ Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+ error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
@ 2020-07-07 16:50 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Eric Blake,
Max Reitz, Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.
It has three goals:
1. Fix issue with error_fatal and error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]
2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]
3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).
To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
[Comments merged properly with recent commit "error: Document Error
API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
before its helpers, and touch up style. Commit message tweaked.]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/error.h | 160 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 141 insertions(+), 19 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3fed49747d..c865a7d2f1 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -30,6 +30,10 @@
* job. Since the value of @errp is about handling the error, the
* function should not examine it.
*
+ * - The function may pass @errp to functions it calls to pass on
+ * their errors to its caller. If it dereferences @errp to check
+ * for errors, it must use ERRP_AUTO_PROPAGATE().
+ *
* - On success, the function should not touch *errp. On failure, it
* should set a new error, e.g. with error_setg(errp, ...), or
* propagate an existing one, e.g. with error_propagate(errp, ...).
@@ -45,15 +49,17 @@
* = Creating errors =
*
* Create an error:
- * error_setg(&err, "situation normal, all fouled up");
+ * error_setg(errp, "situation normal, all fouled up");
+ * where @errp points to the location to receive the error.
*
* Create an error and add additional explanation:
- * error_setg(&err, "invalid quark");
- * error_append_hint(&err, "Valid quarks are up, down, strange, "
+ * error_setg(errp, "invalid quark");
+ * error_append_hint(errp, "Valid quarks are up, down, strange, "
* "charm, top, bottom.\n");
+ * This may require use of ERRP_AUTO_PROPAGATE(); more on that below.
*
* Do *not* contract this to
- * error_setg(&err, "invalid quark\n" // WRONG!
+ * error_setg(errp, "invalid quark\n" // WRONG!
* "Valid quarks are up, down, strange, charm, top, bottom.");
*
* = Reporting and destroying errors =
@@ -107,18 +113,6 @@
* Errors get passed to the caller through the conventional @errp
* parameter.
*
- * Pass an existing error to the caller:
- * error_propagate(errp, err);
- * where Error **errp is a parameter, by convention the last one.
- *
- * Pass an existing error to the caller with the message modified:
- * error_propagate_prepend(errp, err,
- * "Could not frobnicate '%s': ", name);
- * This is more concise than
- * error_propagate(errp, err); // don't do this
- * error_prepend(errp, "Could not frobnicate '%s': ", name);
- * and works even when @errp is &error_fatal.
- *
* Create a new error and pass it to the caller:
* error_setg(errp, "situation normal, all fouled up");
*
@@ -128,18 +122,26 @@
* handle the error...
* }
* when it doesn't, say a void function:
+ * ERRP_AUTO_PROPAGATE();
+ * foo(arg, errp);
+ * if (*errp) {
+ * handle the error...
+ * }
+ * More on ERRP_AUTO_PROPAGATE() below.
+ *
+ * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
- * error_propagate(errp, err);
+ * error_propagate(errp, err); // deprecated
* }
- * Do *not* "optimize" this to
+ * Avoid in new code. Do *not* "optimize" it to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
- * because errp may be NULL!
+ * because errp may be NULL! Guard with ERRP_AUTO_PROPAGATE().
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
@@ -158,6 +160,19 @@
* handle the error...
* }
*
+ * Pass an existing error to the caller:
+ * error_propagate(errp, err);
+ * This is rarely needed. When @err is a local variable, use of
+ * ERRP_AUTO_PROPAGATE() commonly results in more readable code.
+ *
+ * Pass an existing error to the caller with the message modified:
+ * error_propagate_prepend(errp, err,
+ * "Could not frobnicate '%s': ", name);
+ * This is more concise than
+ * error_propagate(errp, err); // don't do this
+ * error_prepend(errp, "Could not frobnicate '%s': ", name);
+ * and works even when @errp is &error_fatal.
+ *
* Receive and accumulate multiple errors (first one wins):
* Error *err = NULL, *local_err = NULL;
* foo(arg, &err);
@@ -185,6 +200,70 @@
* error_setg(&err, ...); // WRONG!
* }
* because this may pass a non-null err to error_setg().
+ *
+ * = Why, when and how to use ERRP_AUTO_PROPAGATE() =
+ *
+ * Without ERRP_AUTO_PROPAGATE(), use of the @errp parameter is
+ * restricted:
+ * - It must not be dereferenced, because it may be null.
+ * - It should not be passed to error_prepend() or
+ * error_append_hint(), because that doesn't work with &error_fatal.
+ * ERRP_AUTO_PROPAGATE() lifts these restrictions.
+ *
+ * To use ERRP_AUTO_PROPAGATE(), add it right at the beginning of the
+ * function. @errp can then be used without worrying about the
+ * argument being NULL or &error_fatal.
+ *
+ * Using it when it's not needed is safe, but please avoid cluttering
+ * the source with useless code.
+ *
+ * = Converting to ERRP_AUTO_PROPAGATE() =
+ *
+ * To convert a function to use ERRP_AUTO_PROPAGATE():
+ *
+ * 0. If the Error ** parameter is not named @errp, rename it to
+ * @errp.
+ *
+ * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
+ * the beginning of the function. This makes @errp safe to use.
+ *
+ * 2. Replace &err by errp, and err by *errp. Delete local variable
+ * @err.
+ *
+ * 3. Delete error_propagate(errp, *errp), replace
+ * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
+ *
+ * 4. Ensure @errp is valid at return: when you destroy *errp, set
+ * errp = NULL.
+ *
+ * Example:
+ *
+ * bool fn(..., Error **errp)
+ * {
+ * Error *err = NULL;
+ *
+ * foo(arg, &err);
+ * if (err) {
+ * handle the error...
+ * error_propagate(errp, err);
+ * return false;
+ * }
+ * ...
+ * }
+ *
+ * becomes
+ *
+ * bool fn(..., Error **errp)
+ * {
+ * ERRP_AUTO_PROPAGATE();
+ *
+ * foo(arg, errp);
+ * if (*errp) {
+ * handle the error...
+ * return false;
+ * }
+ * ...
+ * }
*/
#ifndef ERROR_H
@@ -285,6 +364,7 @@ void error_setg_win32_internal(Error **errp,
* the error object.
* Else, move the error object from @local_err to *@dst_errp.
* On return, @local_err is invalid.
+ * Please use ERRP_AUTO_PROPAGATE() instead when possible.
* Please don't error_propagate(&error_fatal, ...), use
* error_report_err() and exit(), because that's more obvious.
*/
@@ -296,6 +376,8 @@ void error_propagate(Error **dst_errp, Error *local_err);
* Behaves like
* error_prepend(&local_err, fmt, ...);
* error_propagate(dst_errp, local_err);
+ * Please use ERRP_AUTO_PROPAGATE() and error_prepend() instead when
+ * possible.
*/
void error_propagate_prepend(Error **dst_errp, Error *local_err,
const char *fmt, ...);
@@ -393,6 +475,46 @@ void error_set_internal(Error **errp,
ErrorClass err_class, const char *fmt, ...)
GCC_FMT_ATTR(6, 7);
+/*
+ * Make @errp parameter easier to use regardless of argument value
+ *
+ * This macro is for use right at the beginning of a function that
+ * takes an Error **errp parameter to pass errors to its caller. The
+ * parameter must be named @errp.
+ *
+ * It must be used when the function dereferences @errp or passes
+ * @errp to error_prepend(), error_vprepend(), or error_append_hint().
+ * It is safe to use even when it's not needed, but please avoid
+ * cluttering the source with useless code.
+ *
+ * If @errp is NULL or &error_fatal, rewrite it to point to a local
+ * Error variable, which will be automatically propagated to the
+ * original @errp on function exit.
+ *
+ * Note: &error_abort is not rewritten, because that would move the
+ * abort from the place where the error is created to the place where
+ * it's propagated.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+ g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+ do { \
+ if (!errp || errp == &error_fatal) { \
+ errp = &_auto_errp_prop.local_err; \
+ } \
+ } while (0)
+
+typedef struct ErrorPropagator {
+ Error *local_err;
+ Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+ error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
2020-07-07 16:50 ` Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 19:36 ` Eric Blake
2020-07-07 16:50 ` [PATCH v12 3/8] sd: Use ERRP_AUTO_PROPAGATE() Markus Armbruster
` (7 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)
Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
--max-width 80 FILES...
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/coccinelle/auto-propagated-errp.cocci | 337 ++++++++++++++++++
include/qapi/error.h | 3 +
MAINTAINERS | 1 +
3 files changed, 341 insertions(+)
create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 0000000000..c29f695adf
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,337 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// Copyright (c) 2020 Virtuozzo International GmbH.
+//
+// This program is free software; you can redistribute it and/or
+// modify it under the terms of the GNU General Public License as
+// published by the Free Software Foundation; either version 2 of the
+// License, or (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program. If not, see
+// <http://www.gnu.org/licenses/>.
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+// --macro-file scripts/cocci-macro-file.h --in-place \
+// --no-show-diff --max-width 80 FILES...
+//
+// Note: --max-width 80 is needed because coccinelle default is less
+// than 80, and without this parameter coccinelle may reindent some
+// lines which fit into 80 characters but not to coccinelle default,
+// which in turn produces extra patch hunks for no reason.
+
+// Switch unusual Error ** parameter names to errp
+// (this is necessary to use ERRP_AUTO_PROPAGATE).
+//
+// Disable optional_qualifier to skip functions with
+// "Error *const *errp" parameter.
+//
+// Skip functions with "assert(_errp && *_errp)" statement, because
+// that signals unusual semantics, and the parameter name may well
+// serve a purpose. (like nbd_iter_channel_error()).
+//
+// Skip util/error.c to not touch, for example, error_propagate() and
+// error_propagate_prepend().
+@ depends on !(file in "util/error.c") disable optional_qualifier@
+identifier fn;
+identifier _errp != errp;
+@@
+
+ fn(...,
+- Error **_errp
++ Error **errp
+ ,...)
+ {
+(
+ ... when != assert(_errp && *_errp)
+&
+ <...
+- _errp
++ errp
+ ...>
+)
+ }
+
+// Add invocation of ERRP_AUTO_PROPAGATE to errp-functions where
+// necessary
+//
+// Note, that without "when any" the final "..." does not mach
+// something matched by previous pattern, i.e. the rule will not match
+// double error_prepend in control flow like in
+// vfio_set_irq_signaling().
+//
+// Note, "exists" says that we want apply rule even if it does not
+// match on all possible control flows (otherwise, it will not match
+// standard pattern when error_propagate() call is in if branch).
+@ disable optional_qualifier exists@
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
++ ERRP_AUTO_PROPAGATE();
+ ... when != ERRP_AUTO_PROPAGATE();
+(
+(
+ error_append_hint(errp, ...);
+|
+ error_prepend(errp, ...);
+|
+ error_vprepend(errp, ...);
+)
+ ... when any
+|
+ Error *local_err = NULL;
+ ...
+(
+ error_propagate_prepend(errp, local_err, ...);
+|
+ error_propagate(errp, local_err);
+)
+ ...
+)
+ }
+
+// Warn when several Error * definitions are in the control flow.
+// This rule is not chained to rule1 and less restrictive, to cover more
+// functions to warn (even those we are not going to convert).
+//
+// Note, that even with one (or zero) Error * definition in the each
+// control flow we may have several (in total) Error * definitions in
+// the function. This case deserves attention too, but I don't see
+// simple way to match with help of coccinelle.
+@check1 disable optional_qualifier exists@
+identifier fn, _errp, local_err, local_err2;
+position p1, p2;
+@@
+
+ fn(..., Error **_errp, ...)
+ {
+ ...
+ Error *local_err = NULL;@p1
+ ... when any
+ Error *local_err2 = NULL;@p2
+ ... when any
+ }
+
+@ script:python @
+fn << check1.fn;
+p1 << check1.p1;
+p2 << check1.p2;
+@@
+
+print('Warning: function {} has several definitions of '
+ 'Error * local variable: at {}:{} and then at {}:{}'.format(
+ fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))
+
+// Warn when several propagations are in the control flow.
+@check2 disable optional_qualifier exists@
+identifier fn, _errp;
+position p1, p2;
+@@
+
+ fn(..., Error **_errp, ...)
+ {
+ ...
+(
+ error_propagate_prepend(_errp, ...);@p1
+|
+ error_propagate(_errp, ...);@p1
+)
+ ...
+(
+ error_propagate_prepend(_errp, ...);@p2
+|
+ error_propagate(_errp, ...);@p2
+)
+ ... when any
+ }
+
+@ script:python @
+fn << check2.fn;
+p1 << check2.p1;
+p2 << check2.p2;
+@@
+
+print('Warning: function {} propagates to errp several times in '
+ 'one control flow: at {}:{} and then at {}:{}'.format(
+ fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))
+
+// Match functions with propagation of local error to errp.
+// We want to refer these functions in several following rules, but I
+// don't know a proper way to inherit a function, not just its name
+// (to not match another functions with same name in following rules).
+// Not-proper way is as follows: rename errp parameter in functions
+// header and match it in following rules. Rename it back after all
+// transformations.
+//
+// The common case is a single definition of local_err with at most one
+// error_propagate_prepend() or error_propagate() on each control-flow
+// path. Functions with multiple definitions or propagates we want to
+// examine manually. Rules check1 and check2 emit warnings to guide us
+// to them.
+//
+// Note that we match not only this "common case", but any function,
+// which has the "common case" on at least one control-flow path.
+@rule1 disable optional_qualifier exists@
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **
+- errp
++ ____
+ , ...)
+ {
+ ...
+ Error *local_err = NULL;
+ ...
+(
+ error_propagate_prepend(errp, local_err, ...);
+|
+ error_propagate(errp, local_err);
+)
+ ...
+ }
+
+// Convert special case with goto separately.
+// I tried merging this into the following rule the obvious way, but
+// it made Coccinelle hang on block.c
+//
+// Note interesting thing: if we don't do it here, and try to fixup
+// "out: }" things later after all transformations (the rule will be
+// the same, just without error_propagate() call), coccinelle fails to
+// match this "out: }".
+@ disable optional_qualifier@
+identifier rule1.fn, rule1.local_err, out;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+ <...
+- goto out;
++ return;
+ ...>
+- out:
+- error_propagate(errp, local_err);
+ }
+
+// Convert most of local_err related stuff.
+//
+// Note, that we inherit rule1.fn and rule1.local_err names, not
+// objects themselves. We may match something not related to the
+// pattern matched by rule1. For example, local_err may be defined with
+// the same name in different blocks inside one function, and in one
+// block follow the propagation pattern and in other block doesn't.
+//
+// Note also that errp-cleaning functions
+// error_free_errp
+// error_report_errp
+// error_reportf_errp
+// warn_report_errp
+// warn_reportf_errp
+// are not yet implemented. They must call corresponding Error* -
+// freeing function and then set *errp to NULL, to avoid further
+// propagation to original errp (consider ERRP_AUTO_PROPAGATE in use).
+// For example, error_free_errp may look like this:
+//
+// void error_free_errp(Error **errp)
+// {
+// error_free(*errp);
+// *errp = NULL;
+// }
+@ disable optional_qualifier exists@
+identifier rule1.fn, rule1.local_err;
+expression list args;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+ <...
+(
+- Error *local_err = NULL;
+|
+
+// Convert error clearing functions
+(
+- error_free(local_err);
++ error_free_errp(errp);
+|
+- error_report_err(local_err);
++ error_report_errp(errp);
+|
+- error_reportf_err(local_err, args);
++ error_reportf_errp(errp, args);
+|
+- warn_report_err(local_err);
++ warn_report_errp(errp);
+|
+- warn_reportf_err(local_err, args);
++ warn_reportf_errp(errp, args);
+)
+?- local_err = NULL;
+
+|
+- error_propagate_prepend(errp, local_err, args);
++ error_prepend(errp, args);
+|
+- error_propagate(errp, local_err);
+|
+- &local_err
++ errp
+)
+ ...>
+ }
+
+// Convert remaining local_err usage. For example, different kinds of
+// error checking in if conditionals. We can't merge this into
+// previous hunk, as this conflicts with other substitutions in it (at
+// least with "- local_err = NULL").
+@ disable optional_qualifier@
+identifier rule1.fn, rule1.local_err;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+ <...
+- local_err
++ *errp
+ ...>
+ }
+
+// Always use the same pattern for checking error
+@ disable optional_qualifier@
+identifier rule1.fn;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+ <...
+- *errp != NULL
++ *errp
+ ...>
+ }
+
+// Revert temporary ___ identifier.
+@ disable optional_qualifier@
+identifier rule1.fn;
+@@
+
+ fn(..., Error **
+- ____
++ errp
+ , ...)
+ {
+ ...
+ }
diff --git a/include/qapi/error.h b/include/qapi/error.h
index c865a7d2f1..91547fe4ea 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -264,6 +264,9 @@
* }
* ...
* }
+ *
+ * For mass-conversion, use script
+ * scripts/coccinelle/auto-propagated-errp.cocci
*/
#ifndef ERROR_H
diff --git a/MAINTAINERS b/MAINTAINERS
index c31c878c63..121953b24d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2166,6 +2166,7 @@ F: scripts/coccinelle/error-use-after-free.cocci
F: scripts/coccinelle/error_propagate_null.cocci
F: scripts/coccinelle/remove_local_err.cocci
F: scripts/coccinelle/use-error_fatal.cocci
+F: scripts/coccinelle/auto-propagated-errp.cocci
GDB stub
M: Alex Bennée <alex.bennee@linaro.org>
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 3/8] sd: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
2020-07-07 16:50 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE() Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 4/8] pflash: " Markus Armbruster
` (6 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^SD (Secure Card)$/,/^$/{s/^F: //p}' \
MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/sd/sdhci-pci.c | 7 +++----
hw/sd/sdhci.c | 21 +++++++++------------
hw/sd/ssi-sd.c | 10 +++++-----
3 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index 4f5977d487..38ec572fc6 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -29,13 +29,12 @@ static Property sdhci_pci_properties[] = {
static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
SDHCIState *s = PCI_SDHCI(dev);
- Error *local_err = NULL;
sdhci_initfn(s);
- sdhci_common_realize(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ sdhci_common_realize(s, errp);
+ if (*errp) {
return;
}
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index eb2be6529e..be1928784d 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1288,7 +1288,7 @@ static const MemoryRegionOps sdhci_mmio_ops = {
static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
{
- Error *local_err = NULL;
+ ERRP_AUTO_PROPAGATE();
switch (s->sd_spec_version) {
case 2 ... 3:
@@ -1299,9 +1299,8 @@ static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
}
s->version = (SDHC_HCVER_VENDOR << 8) | (s->sd_spec_version - 1);
- sdhci_check_capareg(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ sdhci_check_capareg(s, errp);
+ if (*errp) {
return;
}
}
@@ -1332,11 +1331,10 @@ void sdhci_uninitfn(SDHCIState *s)
void sdhci_common_realize(SDHCIState *s, Error **errp)
{
- Error *local_err = NULL;
+ ERRP_AUTO_PROPAGATE();
- sdhci_init_readonly_registers(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ sdhci_init_readonly_registers(s, errp);
+ if (*errp) {
return;
}
s->buf_maxsz = sdhci_get_fifolen(s);
@@ -1456,13 +1454,12 @@ static void sdhci_sysbus_finalize(Object *obj)
static void sdhci_sysbus_realize(DeviceState *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
SDHCIState *s = SYSBUS_SDHCI(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
- Error *local_err = NULL;
- sdhci_common_realize(s, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ sdhci_common_realize(s, errp);
+ if (*errp) {
return;
}
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 3717b2e721..adb7fa9c24 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -241,10 +241,10 @@ static const VMStateDescription vmstate_ssi_sd = {
static void ssi_sd_realize(SSISlave *d, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
ssi_sd_state *s = SSI_SD(d);
DeviceState *carddev;
DriveInfo *dinfo;
- Error *err = NULL;
qbus_create_inplace(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS,
DEVICE(d), "sd-bus");
@@ -255,23 +255,23 @@ static void ssi_sd_realize(SSISlave *d, Error **errp)
carddev = qdev_new(TYPE_SD_CARD);
if (dinfo) {
if (!qdev_prop_set_drive_err(carddev, "drive",
- blk_by_legacy_dinfo(dinfo), &err)) {
+ blk_by_legacy_dinfo(dinfo), errp)) {
goto fail;
}
}
- if (!object_property_set_bool(OBJECT(carddev), "spi", true, &err)) {
+ if (!object_property_set_bool(OBJECT(carddev), "spi", true, errp)) {
goto fail;
}
- if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
+ if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), errp)) {
goto fail;
}
return;
fail:
- error_propagate_prepend(errp, err, "failed to init SD card: ");
+ error_prepend(errp, "failed to init SD card: ");
}
static void ssi_sd_reset(DeviceState *dev)
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 4/8] pflash: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (2 preceding siblings ...)
2020-07-07 16:50 ` [PATCH v12 3/8] sd: Use ERRP_AUTO_PROPAGATE() Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 5/8] fw_cfg: " Markus Armbruster
` (5 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^Parallel NOR Flash devices$/,/^$/{s/^F: //p}' \
MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/block/pflash_cfi01.c | 7 +++----
hw/block/pflash_cfi02.c | 7 +++----
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index cddc3a5a0c..859cfeae14 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -696,12 +696,12 @@ static const MemoryRegionOps pflash_cfi01_ops = {
static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
PFlashCFI01 *pfl = PFLASH_CFI01(dev);
uint64_t total_len;
int ret;
uint64_t blocks_per_device, sector_len_per_device, device_len;
int num_devices;
- Error *local_err = NULL;
if (pfl->sector_len == 0) {
error_setg(errp, "attribute \"sector-length\" not specified or zero.");
@@ -735,9 +735,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
&pfl->mem, OBJECT(dev),
&pflash_cfi01_ops,
pfl,
- pfl->name, total_len, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ pfl->name, total_len, errp);
+ if (*errp) {
return;
}
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index b40ce2335a..15035ee5ef 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -724,9 +724,9 @@ static const MemoryRegionOps pflash_cfi02_ops = {
static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
PFlashCFI02 *pfl = PFLASH_CFI02(dev);
int ret;
- Error *local_err = NULL;
if (pfl->uniform_sector_len == 0 && pfl->sector_len[0] == 0) {
error_setg(errp, "attribute \"sector-length\" not specified or zero.");
@@ -792,9 +792,8 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
&pflash_cfi02_ops, pfl, pfl->name,
- pfl->chip_len, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ pfl->chip_len, errp);
+ if (*errp) {
return;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 5/8] fw_cfg: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (3 preceding siblings ...)
2020-07-07 16:50 ` [PATCH v12 4/8] pflash: " Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 6/8] virtio-9p: " Markus Armbruster
` (4 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^Firmware configuration (fw_cfg)$/,/^$/{s/^F: //p}' \
MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/nvram/fw_cfg.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0408a31f8e..d5386c3235 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1231,12 +1231,11 @@ static Property fw_cfg_io_properties[] = {
static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
FWCfgIoState *s = FW_CFG_IO(dev);
- Error *local_err = NULL;
- fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ fw_cfg_file_slots_allocate(FW_CFG(s), errp);
+ if (*errp) {
return;
}
@@ -1282,14 +1281,13 @@ static Property fw_cfg_mem_properties[] = {
static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
FWCfgMemState *s = FW_CFG_MEM(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
- Error *local_err = NULL;
- fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ fw_cfg_file_slots_allocate(FW_CFG(s), errp);
+ if (*errp) {
return;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 6/8] virtio-9p: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (4 preceding siblings ...)
2020-07-07 16:50 ` [PATCH v12 5/8] fw_cfg: " Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 7/8] nbd: " Markus Armbruster
` (3 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^virtio-9p$/,/^$/{s/^F: //p}' MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Acked-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/9pfs/9p-local.c | 12 +++++-------
hw/9pfs/9p.c | 1 +
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 54e012e5b4..0361e0c0b4 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1479,10 +1479,10 @@ static void error_append_security_model_hint(Error *const *errp)
static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
const char *sec_model = qemu_opt_get(opts, "security_model");
const char *path = qemu_opt_get(opts, "path");
const char *multidevs = qemu_opt_get(opts, "multidevs");
- Error *local_err = NULL;
if (!sec_model) {
error_setg(errp, "security_model property not set");
@@ -1516,11 +1516,10 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
fse->export_flags &= ~V9FS_FORBID_MULTIDEVS;
fse->export_flags &= ~V9FS_REMAP_INODES;
} else {
- error_setg(&local_err, "invalid multidevs property '%s'",
+ error_setg(errp, "invalid multidevs property '%s'",
multidevs);
- error_append_hint(&local_err, "Valid options are: multidevs="
+ error_append_hint(errp, "Valid options are: multidevs="
"[remap|forbid|warn]\n");
- error_propagate(errp, local_err);
return -1;
}
}
@@ -1530,9 +1529,8 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
return -1;
}
- if (fsdev_throttle_parse_opts(opts, &fse->fst, &local_err)) {
- error_propagate_prepend(errp, local_err,
- "invalid throttle configuration: ");
+ if (fsdev_throttle_parse_opts(opts, &fse->fst, errp)) {
+ error_prepend(errp, "invalid throttle configuration: ");
return -1;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 9755fba9a9..bdb1360482 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -4011,6 +4011,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr)
int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
int i, len;
struct stat stat;
FsDriverEntry *fse;
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 7/8] nbd: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (5 preceding siblings ...)
2020-07-07 16:50 ` [PATCH v12 6/8] virtio-9p: " Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 19:43 ` Eric Blake
2020-07-07 16:50 ` [PATCH v12 8/8] xen: " Markus Armbruster
` (2 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \
MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/block/nbd.h | 1 +
block/nbd.c | 7 +++----
nbd/client.c | 5 +++++
nbd/server.c | 5 +++++
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 20363280ae..f7d87636d3 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -361,6 +361,7 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp);
static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
const char *desc, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
if (ret < 0) {
diff --git a/block/nbd.c b/block/nbd.c
index 6876da04a7..b7cea0f650 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1408,16 +1408,15 @@ static void nbd_client_close(BlockDriverState *bs)
static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
QIOChannelSocket *sioc;
- Error *local_err = NULL;
sioc = qio_channel_socket_new();
qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client");
- qio_channel_socket_connect_sync(sioc, saddr, &local_err);
- if (local_err) {
+ qio_channel_socket_connect_sync(sioc, saddr, errp);
+ if (*errp) {
object_unref(OBJECT(sioc));
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/nbd/client.c b/nbd/client.c
index ba173108ba..e258ef3f7e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -68,6 +68,7 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
uint32_t len, const char *data,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
NBDOption req;
QEMU_BUILD_BUG_ON(sizeof(req) != 16);
@@ -153,6 +154,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
bool strict, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
g_autofree char *msg = NULL;
if (!(reply->type & (1 << 31))) {
@@ -337,6 +339,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
NBDExportInfo *info, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
NBDOptionReply reply;
uint32_t len = strlen(info->name);
uint16_t type;
@@ -882,6 +885,7 @@ static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
bool structured_reply, bool *zeroes,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
uint64_t magic;
trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
@@ -1017,6 +1021,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
const char *hostname, QIOChannel **outioc,
NBDExportInfo *info, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
int result;
bool zeroes;
bool base_allocation = info->base_allocation;
diff --git a/nbd/server.c b/nbd/server.c
index 20754e9ebc..8a12e586d7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -211,6 +211,7 @@ static int GCC_FMT_ATTR(4, 0)
nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
Error **errp, const char *fmt, va_list va)
{
+ ERRP_AUTO_PROPAGATE();
g_autofree char *msg = NULL;
int ret;
size_t len;
@@ -382,6 +383,7 @@ static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
size_t name_len, desc_len;
uint32_t len;
const char *name = exp->name ? exp->name : "";
@@ -445,6 +447,7 @@ static void nbd_check_meta_export(NBDClient *client)
static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
g_autofree char *name = NULL;
char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
size_t len;
@@ -1289,6 +1292,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
*/
static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";
int ret;
@@ -1663,6 +1667,7 @@ void nbd_export_close(NBDExport *exp)
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
nbd_export_close(exp);
return;
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v12 8/8] xen: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (6 preceding siblings ...)
2020-07-07 16:50 ` [PATCH v12 7/8] nbd: " Markus Armbruster
@ 2020-07-07 16:50 ` Markus Armbruster
2020-07-07 18:47 ` [PATCH v12 0/8] error: auto propagated local_err part I Vladimir Sementsov-Ogievskiy
2020-07-07 18:52 ` Eric Blake
9 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 16:50 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^X86 Xen CPUs$/,/^$/{s/^F: //p}' MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/block/dataplane/xen-block.c | 17 +++---
hw/block/xen-block.c | 102 ++++++++++++++-------------------
hw/pci-host/xen_igd_pt.c | 7 +--
hw/xen/xen-backend.c | 7 +--
hw/xen/xen-bus.c | 92 +++++++++++++----------------
hw/xen/xen-host-pci-device.c | 27 +++++----
hw/xen/xen_pt.c | 25 ++++----
hw/xen/xen_pt_config_init.c | 17 +++---
8 files changed, 128 insertions(+), 166 deletions(-)
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 5f8f15778b..1a077cc05f 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -723,8 +723,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
unsigned int protocol,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenDevice *xendev = dataplane->xendev;
- Error *local_err = NULL;
unsigned int ring_size;
unsigned int i;
@@ -760,9 +760,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
}
xen_device_set_max_grant_refs(xendev, dataplane->nr_ring_ref,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (*errp) {
goto stop;
}
@@ -770,9 +769,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
dataplane->ring_ref,
dataplane->nr_ring_ref,
PROT_READ | PROT_WRITE,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (*errp) {
goto stop;
}
@@ -805,9 +803,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
dataplane->event_channel =
xen_device_bind_event_channel(xendev, event_channel,
xen_block_dataplane_event, dataplane,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (*errp) {
goto stop;
}
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a775fba7c0..623ae5b8e0 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -195,6 +195,7 @@ static const BlockDevOps xen_block_dev_ops = {
static void xen_block_realize(XenDevice *xendev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
XenBlockDeviceClass *blockdev_class =
XEN_BLOCK_DEVICE_GET_CLASS(xendev);
@@ -202,7 +203,6 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
XenBlockVdev *vdev = &blockdev->props.vdev;
BlockConf *conf = &blockdev->props.conf;
BlockBackend *blk = conf->blk;
- Error *local_err = NULL;
if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
error_setg(errp, "vdev property not set");
@@ -212,9 +212,8 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
trace_xen_block_realize(type, vdev->disk, vdev->partition);
if (blockdev_class->realize) {
- blockdev_class->realize(blockdev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ blockdev_class->realize(blockdev, errp);
+ if (*errp) {
return;
}
}
@@ -280,8 +279,8 @@ static void xen_block_frontend_changed(XenDevice *xendev,
enum xenbus_state frontend_state,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
- Error *local_err = NULL;
switch (frontend_state) {
case XenbusStateInitialised:
@@ -290,15 +289,13 @@ static void xen_block_frontend_changed(XenDevice *xendev,
break;
}
- xen_block_disconnect(xendev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xen_block_disconnect(xendev, errp);
+ if (*errp) {
break;
}
- xen_block_connect(xendev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xen_block_connect(xendev, errp);
+ if (*errp) {
break;
}
@@ -311,9 +308,8 @@ static void xen_block_frontend_changed(XenDevice *xendev,
case XenbusStateClosed:
case XenbusStateUnknown:
- xen_block_disconnect(xendev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xen_block_disconnect(xendev, errp);
+ if (*errp) {
break;
}
@@ -665,9 +661,9 @@ static void xen_block_blockdev_del(const char *node_name, Error **errp)
static char *xen_block_blockdev_add(const char *id, QDict *qdict,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
const char *driver = qdict_get_try_str(qdict, "driver");
BlockdevOptions *options = NULL;
- Error *local_err = NULL;
char *node_name;
Visitor *v;
@@ -688,10 +684,9 @@ static char *xen_block_blockdev_add(const char *id, QDict *qdict,
goto fail;
}
- qmp_blockdev_add(options, &local_err);
+ qmp_blockdev_add(options, errp);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (*errp) {
goto fail;
}
@@ -710,14 +705,12 @@ fail:
static void xen_block_drive_destroy(XenBlockDrive *drive, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
char *node_name = drive->node_name;
if (node_name) {
- Error *local_err = NULL;
-
- xen_block_blockdev_del(node_name, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xen_block_blockdev_del(node_name, errp);
+ if (*errp) {
return;
}
g_free(node_name);
@@ -731,6 +724,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
const char *device_type,
QDict *opts, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
const char *params = qdict_get_try_str(opts, "params");
const char *mode = qdict_get_try_str(opts, "mode");
const char *direct_io_safe = qdict_get_try_str(opts, "direct-io-safe");
@@ -738,7 +732,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
char *driver = NULL;
char *filename = NULL;
XenBlockDrive *drive = NULL;
- Error *local_err = NULL;
QDict *file_layer;
QDict *driver_layer;
@@ -817,13 +810,12 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
g_assert(!drive->node_name);
drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
- &local_err);
+ errp);
qobject_unref(driver_layer);
done:
- if (local_err) {
- error_propagate(errp, local_err);
+ if (*errp) {
xen_block_drive_destroy(drive, NULL);
return NULL;
}
@@ -848,8 +840,8 @@ static void xen_block_iothread_destroy(XenBlockIOThread *iothread,
static XenBlockIOThread *xen_block_iothread_create(const char *id,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
- Error *local_err = NULL;
QDict *opts;
QObject *ret_data = NULL;
@@ -858,13 +850,11 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id,
opts = qdict_new();
qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
qdict_put_str(opts, "id", id);
- qmp_object_add(opts, &ret_data, &local_err);
+ qmp_object_add(opts, &ret_data, errp);
qobject_unref(opts);
qobject_unref(ret_data);
- if (local_err) {
- error_propagate(errp, local_err);
-
+ if (*errp) {
g_free(iothread->id);
g_free(iothread);
return NULL;
@@ -876,6 +866,7 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id,
static void xen_block_device_create(XenBackendInstance *backend,
QDict *opts, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenBus *xenbus = xen_backend_get_bus(backend);
const char *name = xen_backend_get_name(backend);
unsigned long number;
@@ -883,7 +874,6 @@ static void xen_block_device_create(XenBackendInstance *backend,
XenBlockDrive *drive = NULL;
XenBlockIOThread *iothread = NULL;
XenDevice *xendev = NULL;
- Error *local_err = NULL;
const char *type;
XenBlockDevice *blockdev;
@@ -915,16 +905,15 @@ static void xen_block_device_create(XenBackendInstance *backend,
goto fail;
}
- drive = xen_block_drive_create(vdev, device_type, opts, &local_err);
+ drive = xen_block_drive_create(vdev, device_type, opts, errp);
if (!drive) {
- error_propagate_prepend(errp, local_err, "failed to create drive: ");
+ error_prepend(errp, "failed to create drive: ");
goto fail;
}
- iothread = xen_block_iothread_create(vdev, &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to create iothread: ");
+ iothread = xen_block_iothread_create(vdev, errp);
+ if (*errp) {
+ error_prepend(errp, "failed to create iothread: ");
goto fail;
}
@@ -932,32 +921,29 @@ static void xen_block_device_create(XenBackendInstance *backend,
blockdev = XEN_BLOCK_DEVICE(xendev);
if (!object_property_set_str(OBJECT(xendev), "vdev", vdev,
- &local_err)) {
- error_propagate_prepend(errp, local_err, "failed to set 'vdev': ");
+ errp)) {
+ error_prepend(errp, "failed to set 'vdev': ");
goto fail;
}
if (!object_property_set_str(OBJECT(xendev), "drive",
xen_block_drive_get_node_name(drive),
- &local_err)) {
- error_propagate_prepend(errp, local_err, "failed to set 'drive': ");
+ errp)) {
+ error_prepend(errp, "failed to set 'drive': ");
goto fail;
}
if (!object_property_set_str(OBJECT(xendev), "iothread", iothread->id,
- &local_err)) {
- error_propagate_prepend(errp, local_err,
- "failed to set 'iothread': ");
+ errp)) {
+ error_prepend(errp, "failed to set 'iothread': ");
goto fail;
}
blockdev->iothread = iothread;
blockdev->drive = drive;
- if (!qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), &local_err)) {
- error_propagate_prepend(errp, local_err,
- "realization of device %s failed: ",
- type);
+ if (!qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
+ error_prepend(errp, "realization of device %s failed: ", type);
goto fail;
}
@@ -981,31 +967,29 @@ fail:
static void xen_block_device_destroy(XenBackendInstance *backend,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenDevice *xendev = xen_backend_get_device(backend);
XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
XenBlockVdev *vdev = &blockdev->props.vdev;
XenBlockDrive *drive = blockdev->drive;
XenBlockIOThread *iothread = blockdev->iothread;
- Error *local_err = NULL;
trace_xen_block_device_destroy(vdev->number);
object_unparent(OBJECT(xendev));
if (iothread) {
- xen_block_iothread_destroy(iothread, &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to destroy iothread: ");
+ xen_block_iothread_destroy(iothread, errp);
+ if (*errp) {
+ error_prepend(errp, "failed to destroy iothread: ");
return;
}
}
if (drive) {
- xen_block_drive_destroy(drive, &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to destroy drive: ");
+ xen_block_drive_destroy(drive, errp);
+ if (*errp) {
+ error_prepend(errp, "failed to destroy drive: ");
return;
}
}
diff --git a/hw/pci-host/xen_igd_pt.c b/hw/pci-host/xen_igd_pt.c
index efcc9347ff..29ade9ca25 100644
--- a/hw/pci-host/xen_igd_pt.c
+++ b/hw/pci-host/xen_igd_pt.c
@@ -79,17 +79,16 @@ static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
uint32_t val = 0;
size_t i;
int pos, len;
- Error *local_err = NULL;
for (i = 0; i < ARRAY_SIZE(igd_host_bridge_infos); i++) {
pos = igd_host_bridge_infos[i].offset;
len = igd_host_bridge_infos[i].len;
- host_pci_config_read(pos, len, &val, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ host_pci_config_read(pos, len, &val, errp);
+ if (*errp) {
return;
}
pci_default_write_config(pci_dev, pos, val, len);
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index da065f81b7..1cc0694053 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -98,9 +98,9 @@ static void xen_backend_list_remove(XenBackendInstance *backend)
void xen_backend_device_create(XenBus *xenbus, const char *type,
const char *name, QDict *opts, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
const XenBackendImpl *impl = xen_backend_table_lookup(type);
XenBackendInstance *backend;
- Error *local_error = NULL;
if (!impl) {
return;
@@ -110,9 +110,8 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
backend->xenbus = xenbus;
backend->name = g_strdup(name);
- impl->create(backend, opts, &local_error);
- if (local_error) {
- error_propagate(errp, local_error);
+ impl->create(backend, opts, errp);
+ if (*errp) {
g_free(backend->name);
g_free(backend);
return;
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index c4e2162ae9..2ea5144ef0 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -53,9 +53,9 @@ static char *xen_device_get_frontend_path(XenDevice *xendev)
static void xen_device_unplug(XenDevice *xendev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
const char *type = object_get_typename(OBJECT(xendev));
- Error *local_err = NULL;
xs_transaction_t tid;
trace_xen_device_unplug(type, xendev->name);
@@ -69,14 +69,14 @@ again:
}
xs_node_printf(xenbus->xsh, tid, xendev->backend_path, "online",
- &local_err, "%u", 0);
- if (local_err) {
+ errp, "%u", 0);
+ if (*errp) {
goto abort;
}
xs_node_printf(xenbus->xsh, tid, xendev->backend_path, "state",
- &local_err, "%u", XenbusStateClosing);
- if (local_err) {
+ errp, "%u", XenbusStateClosing);
+ if (*errp) {
goto abort;
}
@@ -96,7 +96,6 @@ abort:
* from ending the transaction.
*/
xs_transaction_end(xenbus->xsh, tid, true);
- error_propagate(errp, local_err);
}
static void xen_bus_print_dev(Monitor *mon, DeviceState *dev, int indent)
@@ -205,15 +204,13 @@ static XenWatch *watch_list_add(XenWatchList *watch_list, const char *node,
const char *key, XenWatchHandler handler,
void *opaque, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenWatch *watch = new_watch(node, key, handler, opaque);
- Error *local_err = NULL;
notifier_list_add(&watch_list->notifiers, &watch->notifier);
- xs_node_watch(watch_list->xsh, node, key, watch->token, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
-
+ xs_node_watch(watch_list->xsh, node, key, watch->token, errp);
+ if (*errp) {
notifier_remove(&watch->notifier);
free_watch(watch);
@@ -255,11 +252,11 @@ static void xen_bus_backend_create(XenBus *xenbus, const char *type,
const char *name, char *path,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
xs_transaction_t tid;
char **key;
QDict *opts;
unsigned int i, n;
- Error *local_err = NULL;
trace_xen_bus_backend_create(type, path);
@@ -314,13 +311,11 @@ again:
return;
}
- xen_backend_device_create(xenbus, type, name, opts, &local_err);
+ xen_backend_device_create(xenbus, type, name, opts, errp);
qobject_unref(opts);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to create '%s' device '%s': ",
- type, name);
+ if (*errp) {
+ error_prepend(errp, "failed to create '%s' device '%s': ", type, name);
}
}
@@ -692,9 +687,9 @@ static void xen_device_remove_watch(XenDevice *xendev, XenWatch *watch,
static void xen_device_backend_create(XenDevice *xendev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
struct xs_permissions perms[2];
- Error *local_err = NULL;
xendev->backend_path = xen_device_get_backend_path(xendev);
@@ -706,30 +701,27 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
g_assert(xenbus->xsh);
xs_node_create(xenbus->xsh, XBT_NULL, xendev->backend_path, perms,
- ARRAY_SIZE(perms), &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to create backend: ");
+ ARRAY_SIZE(perms), errp);
+ if (*errp) {
+ error_prepend(errp, "failed to create backend: ");
return;
}
xendev->backend_state_watch =
xen_device_add_watch(xendev, xendev->backend_path,
"state", xen_device_backend_changed,
- &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to watch backend state: ");
+ errp);
+ if (*errp) {
+ error_prepend(errp, "failed to watch backend state: ");
return;
}
xendev->backend_online_watch =
xen_device_add_watch(xendev, xendev->backend_path,
"online", xen_device_backend_changed,
- &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to watch backend online: ");
+ errp);
+ if (*errp) {
+ error_prepend(errp, "failed to watch backend online: ");
return;
}
}
@@ -866,9 +858,9 @@ static bool xen_device_frontend_exists(XenDevice *xendev)
static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
struct xs_permissions perms[2];
- Error *local_err = NULL;
xendev->frontend_path = xen_device_get_frontend_path(xendev);
@@ -885,20 +877,18 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
g_assert(xenbus->xsh);
xs_node_create(xenbus->xsh, XBT_NULL, xendev->frontend_path, perms,
- ARRAY_SIZE(perms), &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to create frontend: ");
+ ARRAY_SIZE(perms), errp);
+ if (*errp) {
+ error_prepend(errp, "failed to create frontend: ");
return;
}
}
xendev->frontend_state_watch =
xen_device_add_watch(xendev, xendev->frontend_path, "state",
- xen_device_frontend_changed, &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to watch frontend state: ");
+ xen_device_frontend_changed, errp);
+ if (*errp) {
+ error_prepend(errp, "failed to watch frontend state: ");
}
}
@@ -1247,11 +1237,11 @@ static void xen_device_exit(Notifier *n, void *data)
static void xen_device_realize(DeviceState *dev, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenDevice *xendev = XEN_DEVICE(dev);
XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
const char *type = object_get_typename(OBJECT(xendev));
- Error *local_err = NULL;
if (xendev->frontend_id == DOMID_INVALID) {
xendev->frontend_id = xen_domid;
@@ -1267,10 +1257,9 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
goto unrealize;
}
- xendev->name = xendev_class->get_name(xendev, &local_err);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "failed to get device name: ");
+ xendev->name = xendev_class->get_name(xendev, errp);
+ if (*errp) {
+ error_prepend(errp, "failed to get device name: ");
goto unrealize;
}
@@ -1293,22 +1282,19 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
xendev->feature_grant_copy =
(xengnttab_grant_copy(xendev->xgth, 0, NULL) == 0);
- xen_device_backend_create(xendev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xen_device_backend_create(xendev, errp);
+ if (*errp) {
goto unrealize;
}
- xen_device_frontend_create(xendev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xen_device_frontend_create(xendev, errp);
+ if (*errp) {
goto unrealize;
}
if (xendev_class->realize) {
- xendev_class->realize(xendev, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ xendev_class->realize(xendev, errp);
+ if (*errp) {
goto unrealize;
}
}
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 1b44dcafaf..02379c341c 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -333,8 +333,8 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
uint8_t bus, uint8_t dev, uint8_t func,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
unsigned int v;
- Error *err = NULL;
d->config_fd = -1;
d->domain = domain;
@@ -342,36 +342,36 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
d->dev = dev;
d->func = func;
- xen_host_pci_config_open(d, &err);
- if (err) {
+ xen_host_pci_config_open(d, errp);
+ if (*errp) {
goto error;
}
- xen_host_pci_get_resource(d, &err);
- if (err) {
+ xen_host_pci_get_resource(d, errp);
+ if (*errp) {
goto error;
}
- xen_host_pci_get_hex_value(d, "vendor", &v, &err);
- if (err) {
+ xen_host_pci_get_hex_value(d, "vendor", &v, errp);
+ if (*errp) {
goto error;
}
d->vendor_id = v;
- xen_host_pci_get_hex_value(d, "device", &v, &err);
- if (err) {
+ xen_host_pci_get_hex_value(d, "device", &v, errp);
+ if (*errp) {
goto error;
}
d->device_id = v;
- xen_host_pci_get_dec_value(d, "irq", &v, &err);
- if (err) {
+ xen_host_pci_get_dec_value(d, "irq", &v, errp);
+ if (*errp) {
goto error;
}
d->irq = v;
- xen_host_pci_get_hex_value(d, "class", &v, &err);
- if (err) {
+ xen_host_pci_get_hex_value(d, "class", &v, errp);
+ if (*errp) {
goto error;
}
d->class_code = v;
@@ -381,7 +381,6 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
return;
error:
- error_propagate(errp, err);
if (d->config_fd >= 0) {
close(d->config_fd);
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index ab84443d5e..baa25eb91a 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -777,12 +777,12 @@ static void xen_pt_destroy(PCIDevice *d) {
static void xen_pt_realize(PCIDevice *d, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
int i, rc = 0;
uint8_t machine_irq = 0, scratch;
uint16_t cmd = 0;
int pirq = XEN_PT_UNASSIGNED_PIRQ;
- Error *err = NULL;
/* register real device */
XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -793,10 +793,9 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
xen_host_pci_device_get(&s->real_device,
s->hostaddr.domain, s->hostaddr.bus,
s->hostaddr.slot, s->hostaddr.function,
- &err);
- if (err) {
- error_append_hint(&err, "Failed to \"open\" the real pci device");
- error_propagate(errp, err);
+ errp);
+ if (*errp) {
+ error_append_hint(errp, "Failed to \"open\" the real pci device");
return;
}
@@ -823,11 +822,10 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
return;
}
- xen_pt_setup_vga(s, &s->real_device, &err);
- if (err) {
- error_append_hint(&err, "Setup VGA BIOS of passthrough"
- " GFX failed");
- error_propagate(errp, err);
+ xen_pt_setup_vga(s, &s->real_device, errp);
+ if (*errp) {
+ error_append_hint(errp, "Setup VGA BIOS of passthrough"
+ " GFX failed");
xen_host_pci_device_put(&s->real_device);
return;
}
@@ -840,10 +838,9 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
xen_pt_register_regions(s, &cmd);
/* reinitialize each config register to be emulated */
- xen_pt_config_init(s, &err);
- if (err) {
- error_append_hint(&err, "PCI Config space initialisation failed");
- error_propagate(errp, err);
+ xen_pt_config_init(s, errp);
+ if (*errp) {
+ error_append_hint(errp, "PCI Config space initialisation failed");
rc = -1;
goto err_out;
}
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index d0d7c720a6..af3fbd1bfb 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -2008,8 +2008,8 @@ static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
int i, rc;
- Error *err = NULL;
QLIST_INIT(&s->reg_grps);
@@ -2067,13 +2067,14 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
/* initialize capability register */
for (j = 0; regs->size != 0; j++, regs++) {
- xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
- if (err) {
- error_append_hint(&err, "Failed to init register %d"
- " offsets 0x%x in grp_type = 0x%x (%d/%zu)", j,
- regs->offset, xen_pt_emu_reg_grps[i].grp_type,
- i, ARRAY_SIZE(xen_pt_emu_reg_grps));
- error_propagate(errp, err);
+ xen_pt_config_reg_init(s, reg_grp_entry, regs, errp);
+ if (*errp) {
+ error_append_hint(errp, "Failed to init register %d"
+ " offsets 0x%x in grp_type = 0x%x (%d/%zu)",
+ j,
+ regs->offset,
+ xen_pt_emu_reg_grps[i].grp_type,
+ i, ARRAY_SIZE(xen_pt_emu_reg_grps));
xen_pt_config_delete(s);
return;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v12 0/8] error: auto propagated local_err part I
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (7 preceding siblings ...)
2020-07-07 16:50 ` [PATCH v12 8/8] xen: " Markus Armbruster
@ 2020-07-07 18:47 ` Vladimir Sementsov-Ogievskiy
2020-07-07 18:52 ` Eric Blake
9 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-07 18:47 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, Stefano Stabellini, Michael Roth, qemu-block,
Paul Durrant, Laszlo Ersek, Christian Schoenebeck, groug,
Max Reitz, Gerd Hoffmann, Stefan Hajnoczi, Anthony Perard,
xen-devel, Philippe Mathieu-Daudé
07.07.2020 19:50, Markus Armbruster wrote:
> To speed things up, I'm taking the liberty to respin Vladimir's series
> with my documentation amendments.
Thank you!
>
> After my documentation work, I'm very much inclined to rename
> ERRP_AUTO_PROPAGATE() to ERRP_GUARD(). The fact that it propagates
> below the hood is detail. What matters to its users is that it lets
> them use @errp more freely. Thoughts?
No objections, if we are making error-propagation to be internal implementation detail, no reason to shout about it in the macro name.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 0/8] error: auto propagated local_err part I
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
` (8 preceding siblings ...)
2020-07-07 18:47 ` [PATCH v12 0/8] error: auto propagated local_err part I Vladimir Sementsov-Ogievskiy
@ 2020-07-07 18:52 ` Eric Blake
9 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2020-07-07 18:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, vsementsov, qemu-block, Paul Durrant,
Philippe Mathieu-Daudé,
Christian Schoenebeck, Michael Roth, groug, Stefano Stabellini,
Gerd Hoffmann, Stefan Hajnoczi, Anthony Perard, xen-devel,
Max Reitz, Laszlo Ersek
On 7/7/20 11:50 AM, Markus Armbruster wrote:
> To speed things up, I'm taking the liberty to respin Vladimir's series
> with my documentation amendments.
>
> After my documentation work, I'm very much inclined to rename
> ERRP_AUTO_PROPAGATE() to ERRP_GUARD(). The fact that it propagates
> below the hood is detail. What matters to its users is that it lets
> them use @errp more freely. Thoughts?
I like it - the shorter name is easier to type.
(The rename is a mechanical change, so if we agree to it, we should do
it up front to minimize the churn to all the functions where we add use
of the macro)
>
> Based-on: Message-Id: <20200707160613.848843-1-armbru@redhat.com>
>
> Available from my public repository https://repo.or.cz/qemu/armbru.git
> on branch error-auto.
>
> v12: (based on "[PATCH v4 00/45] Less clumsy error checking")
> 01: Comments merged properly with recent commit "error: Document Error
> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
> before its helpers, and touch up style.
> 01-08: Commit messages tweaked
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 ` Markus Armbruster
(?)
@ 2020-07-07 19:02 ` Eric Blake
2020-07-07 20:01 ` Markus Armbruster
-1 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2020-07-07 19:02 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, vsementsov, Michael Roth, qemu-block, Paul Durrant,
Laszlo Ersek, Christian Schoenebeck, groug, Max Reitz,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
On 7/7/20 11:50 AM, Markus Armbruster wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
the user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> [Comments merged properly with recent commit "error: Document Error
> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
> before its helpers, and touch up style. Commit message tweaked.]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/error.h | 160 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 141 insertions(+), 19 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3fed49747d..c865a7d2f1 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -128,18 +122,26 @@
> * handle the error...
> * }
> * when it doesn't, say a void function:
> + * ERRP_AUTO_PROPAGATE();
> + * foo(arg, errp);
> + * if (*errp) {
> + * handle the error...
> + * }
> + * More on ERRP_AUTO_PROPAGATE() below.
> + *
> + * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:
exists
> * Error *err = NULL;
> * foo(arg, &err);
> * if (err) {
> * handle the error...
> - * error_propagate(errp, err);
> + * error_propagate(errp, err); // deprecated
> * }
> - * Do *not* "optimize" this to
> + * Avoid in new code. Do *not* "optimize" it to
> * foo(arg, errp);
> * if (*errp) { // WRONG!
> * handle the error...
> * }
> - * because errp may be NULL!
> + * because errp may be NULL! Guard with ERRP_AUTO_PROPAGATE().
maybe:
because errp may be NULL without the ERRP_AUTO_PROPAGATE() guard.
> *
> * But when all you do with the error is pass it on, please use
> * foo(arg, errp);
> @@ -158,6 +160,19 @@
> * handle the error...
> * }
> *
> + * Pass an existing error to the caller:
> + * = Converting to ERRP_AUTO_PROPAGATE() =
> + *
> + * To convert a function to use ERRP_AUTO_PROPAGATE():
> + *
> + * 0. If the Error ** parameter is not named @errp, rename it to
> + * @errp.
> + *
> + * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
> + * the beginning of the function. This makes @errp safe to use.
> + *
> + * 2. Replace &err by errp, and err by *errp. Delete local variable
> + * @err.
> + *
> + * 3. Delete error_propagate(errp, *errp), replace
> + * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
> + *
Why a comma here?
> + * 4. Ensure @errp is valid at return: when you destroy *errp, set
> + * errp = NULL.
> + *
> + * Example:
> + *
> + * bool fn(..., Error **errp)
> + * {
> + * Error *err = NULL;
> + *
> + * foo(arg, &err);
> + * if (err) {
> + * handle the error...
> + * error_propagate(errp, err);
> + * return false;
> + * }
> + * ...
> + * }
> + *
> + * becomes
> + *
> + * bool fn(..., Error **errp)
> + * {
> + * ERRP_AUTO_PROPAGATE();
> + *
> + * foo(arg, errp);
> + * if (*errp) {
> + * handle the error...
> + * return false;
> + * }
> + * ...
> + * }
Do we want the example to show the use of error_free and *errp = NULL?
Otherwise, this is looking good to me. It will need a tweak if we go
with the shorter name ERRP_GUARD, but I like that idea.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 ` [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE() Markus Armbruster
@ 2020-07-07 19:36 ` Eric Blake
2020-07-07 20:11 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: Eric Blake @ 2020-07-07 19:36 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, vsementsov, qemu-block, Paul Durrant,
Philippe Mathieu-Daudé,
Christian Schoenebeck, Michael Roth, groug, Stefano Stabellini,
Gerd Hoffmann, Stefan Hajnoczi, Anthony Perard, xen-devel,
Max Reitz, Laszlo Ersek
On 7/7/20 11:50 AM, Markus Armbruster wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
> does corresponding changes in code (look for details in
> include/qapi/error.h)
>
> Usage example:
> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
> --max-width 80 FILES...
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/coccinelle/auto-propagated-errp.cocci | 337 ++++++++++++++++++
> include/qapi/error.h | 3 +
> MAINTAINERS | 1 +
> 3 files changed, 341 insertions(+)
> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
Needs a tweak if we go with ERRP_GUARD. But that's easy.
> +
> +// Convert special case with goto separately.
> +// I tried merging this into the following rule the obvious way, but
> +// it made Coccinelle hang on block.c
> +//
> +// Note interesting thing: if we don't do it here, and try to fixup
> +// "out: }" things later after all transformations (the rule will be
> +// the same, just without error_propagate() call), coccinelle fails to
> +// match this "out: }".
"out: }" is not valid C; would referring to "out: ; }" fare any better?
> +@ disable optional_qualifier@
> +identifier rule1.fn, rule1.local_err, out;
> +symbol errp;
> +@@
> +
> + fn(..., Error ** ____, ...)
> + {
> + <...
> +- goto out;
> ++ return;
> + ...>
> +- out:
> +- error_propagate(errp, local_err);
> + }
> +
> +// Convert most of local_err related stuff.
> +//
> +// Note, that we inherit rule1.fn and rule1.local_err names, not
> +// objects themselves. We may match something not related to the
> +// pattern matched by rule1. For example, local_err may be defined with
> +// the same name in different blocks inside one function, and in one
> +// block follow the propagation pattern and in other block doesn't.
> +//
> +// Note also that errp-cleaning functions
> +// error_free_errp
> +// error_report_errp
> +// error_reportf_errp
> +// warn_report_errp
> +// warn_reportf_errp
> +// are not yet implemented. They must call corresponding Error* -
> +// freeing function and then set *errp to NULL, to avoid further
> +// propagation to original errp (consider ERRP_AUTO_PROPAGATE in use).
> +// For example, error_free_errp may look like this:
> +//
> +// void error_free_errp(Error **errp)
> +// {
> +// error_free(*errp);
> +// *errp = NULL;
> +// }
I guess we can still decide later if we want these additional functions,
or if they will even help after the number of places we have already
improved after applying this script as-is and with Markus' cleanups in
place.
While I won't call myself a Coccinelle expert, it at least looks sane
enough that I'm comfortable if you add:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 7/8] nbd: Use ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 ` [PATCH v12 7/8] nbd: " Markus Armbruster
@ 2020-07-07 19:43 ` Eric Blake
0 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2020-07-07 19:43 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, vsementsov, qemu-block, Paul Durrant,
Philippe Mathieu-Daudé,
Christian Schoenebeck, Michael Roth, groug, Stefano Stabellini,
Gerd Hoffmann, Stefan Hajnoczi, Anthony Perard, xen-devel,
Max Reitz, Laszlo Ersek
On 7/7/20 11:50 AM, Markus Armbruster wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> If we want to add some info to errp (by error_prepend() or
> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
> Otherwise, this info will not be added when errp == &error_fatal
> (the program will exit prior to the error_append_hint() or
> error_prepend() call). Fix such cases.
>
> If we want to check error after errp-function call, we need to
> introduce local_err and then propagate it to errp. Instead, use
> ERRP_AUTO_PROPAGATE macro, benefits are:
> 1. No need of explicit error_propagate call
> 2. No need of explicit local_err variable: use errp directly
> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
> &error_fatal, this means that we don't break error_abort
> (we'll abort on error_set, not on error_propagate)
>
> This commit is generated by command
>
> sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \
> MAINTAINERS | \
> xargs git ls-files | grep '\.[hc]$' | \
> xargs spatch \
> --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> --macro-file scripts/cocci-macro-file.h \
> --in-place --no-show-diff --max-width 80
>
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> [Commit message tweaked]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
2020-07-07 19:02 ` Eric Blake
@ 2020-07-07 20:01 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 20:01 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, vsementsov, qemu-block, Paul Durrant,
Philippe Mathieu-Daudé,
Christian Schoenebeck, qemu-devel, Michael Roth, groug,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Max Reitz, Laszlo Ersek
Eric Blake <eblake@redhat.com> writes:
> On 7/7/20 11:50 AM, Markus Armbruster wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with an errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
>
> the user
Yes.
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort and error_propagate: when we wrap
>> error_abort by local_err+error_propagate, the resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
>> the local_err+error_propagate pattern, which will definitely fix the
>> issue) [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, later patches will add invocations
>> of this macro at the start of functions with either use
>> error_prepend/error_append_hint (solving 1) or which use
>> local_err+error_propagate to check errors, switching those
>> functions to use *errp instead (solving 2 and 3).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Paul Durrant <paul@xen.org>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> [Comments merged properly with recent commit "error: Document Error
>> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
>> before its helpers, and touch up style. Commit message tweaked.]
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/error.h | 160 ++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 141 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3fed49747d..c865a7d2f1 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>
>> @@ -128,18 +122,26 @@
>> * handle the error...
>> * }
>> * when it doesn't, say a void function:
>> + * ERRP_AUTO_PROPAGATE();
>> + * foo(arg, errp);
>> + * if (*errp) {
>> + * handle the error...
>> + * }
>> + * More on ERRP_AUTO_PROPAGATE() below.
>> + *
>> + * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:
>
> exists
Fixing...
>> * Error *err = NULL;
>> * foo(arg, &err);
>> * if (err) {
>> * handle the error...
>> - * error_propagate(errp, err);
>> + * error_propagate(errp, err); // deprecated
>> * }
>> - * Do *not* "optimize" this to
>> + * Avoid in new code. Do *not* "optimize" it to
>> * foo(arg, errp);
>> * if (*errp) { // WRONG!
>> * handle the error...
>> * }
>> - * because errp may be NULL!
>> + * because errp may be NULL! Guard with ERRP_AUTO_PROPAGATE().
>
> maybe:
>
> because errp may be NULL without the ERRP_AUTO_PROPAGATE() guard.
Sold.
>> *
>> * But when all you do with the error is pass it on, please use
>> * foo(arg, errp);
>> @@ -158,6 +160,19 @@
>> * handle the error...
>> * }
>> *
>> + * Pass an existing error to the caller:
>
>> + * = Converting to ERRP_AUTO_PROPAGATE() =
>> + *
>> + * To convert a function to use ERRP_AUTO_PROPAGATE():
>> + *
>> + * 0. If the Error ** parameter is not named @errp, rename it to
>> + * @errp.
>> + *
>> + * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
>> + * the beginning of the function. This makes @errp safe to use.
>> + *
>> + * 2. Replace &err by errp, and err by *errp. Delete local variable
>> + * @err.
>> + *
>> + * 3. Delete error_propagate(errp, *errp), replace
>> + * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
>> + *
>
> Why a comma here?
Editing accident.
>> + * 4. Ensure @errp is valid at return: when you destroy *errp, set
>> + * errp = NULL.
>> + *
>> + * Example:
>> + *
>> + * bool fn(..., Error **errp)
>> + * {
>> + * Error *err = NULL;
>> + *
>> + * foo(arg, &err);
>> + * if (err) {
>> + * handle the error...
>> + * error_propagate(errp, err);
>> + * return false;
>> + * }
>> + * ...
>> + * }
>> + *
>> + * becomes
>> + *
>> + * bool fn(..., Error **errp)
>> + * {
>> + * ERRP_AUTO_PROPAGATE();
>> + *
>> + * foo(arg, errp);
>> + * if (*errp) {
>> + * handle the error...
>> + * return false;
>> + * }
>> + * ...
>> + * }
>
> Do we want the example to show the use of error_free and *errp = NULL?
Yes, but we're running out of time, so let's do it in the series that
introduces the usage to the code.
> Otherwise, this is looking good to me. It will need a tweak if we go
> with the shorter name ERRP_GUARD, but I like that idea.
Tweaking away...
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
2020-07-07 16:50 ` Markus Armbruster
@ 2020-07-07 20:08 ` Vladimir Sementsov-Ogievskiy
-1 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-07 20:08 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, Stefano Stabellini, Michael Roth, qemu-block,
Paul Durrant, Laszlo Ersek, Christian Schoenebeck, groug,
Max Reitz, Gerd Hoffmann, Stefan Hajnoczi, Anthony Perard,
xen-devel, Philippe Mathieu-Daudé
07.07.2020 19:50, Markus Armbruster wrote:
> From: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>
> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> Reviewed-by: Paul Durrant<paul@xen.org>
> Reviewed-by: Greg Kurz<groug@kaod.org>
> Reviewed-by: Eric Blake<eblake@redhat.com>
> [Comments merged properly with recent commit "error: Document Error
> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
> before its helpers, and touch up style. Commit message tweaked.]
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
Ok, I see you have mostly rewritten the big comment and not only in this patch, so, I go and read the whole comment on top of these series.
=================================
* Pass an existing error to the caller with the message modified:
* error_propagate_prepend(errp, err,
* "Could not frobnicate '%s': ", name);
* This is more concise than
* error_propagate(errp, err); // don't do this
* error_prepend(errp, "Could not frobnicate '%s': ", name);
* and works even when @errp is &error_fatal.
- the latter doesn't consider ERRP_AUTO_PROPAGATE: as we know, that ERRP_AUTO_PROPAGATE should be used when we use error_prepend, the latter should look like
ERRP_AUTO_PROPAGATE();
...
error_propagate(errp, err); // don't do this
error_prepend(errp, "Could not frobnicate '%s': ", name);
- and it works even when @errp is &error_fatal, so the error_propagate_prepend now is just a shortcut, not the only correct way.
Still, the text is formally correct as is, and may be improved later.
=================================
* 2. Replace &err by errp, and err by *errp. Delete local variable
* @err.
- hmm a bit not obvious,, It can be local_err. It can be (in some rare cases) still needed to handle the error locally, not passing to the caller..
may be just something like "Assume local Error *err variable is used to get errors from called functions and than propagated to caller's errp" before paragraph [2.] will help.
*
* 3. Delete error_propagate(errp, *errp), replace
* error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
*
* 4. Ensure @errp is valid at return: when you destroy *errp, set
* errp = NULL.
=================================
May be good to add note about ERRP_AUTO_PROPAGATE() into comment above error_append_hint (and error_(v)prepend)).
=================================
/*
* Make @errp parameter easier to use regardless of argument value
may be s/argument/its/
*
* This macro is for use right at the beginning of a function that
* takes an Error **errp parameter to pass errors to its caller. The
* parameter must be named @errp.
*
* It must be used when the function dereferences @errp or passes
* @errp to error_prepend(), error_vprepend(), or error_append_hint().
* It is safe to use even when it's not needed, but please avoid
* cluttering the source with useless code.
*
* If @errp is NULL or &error_fatal, rewrite it to point to a local
* Error variable, which will be automatically propagated to the
* original @errp on function exit.
*
* Note: &error_abort is not rewritten, because that would move the
* abort from the place where the error is created to the place where
* it's propagated.
*/
=================================
All these are minor, the documentation is good as is, thank you!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
@ 2020-07-07 20:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-07 20:08 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Kevin Wolf, Stefano Stabellini, Michael Roth, qemu-block,
Paul Durrant, Laszlo Ersek, Christian Schoenebeck, groug,
Eric Blake, Max Reitz, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Philippe Mathieu-Daudé
07.07.2020 19:50, Markus Armbruster wrote:
> From: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>
> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> Reviewed-by: Paul Durrant<paul@xen.org>
> Reviewed-by: Greg Kurz<groug@kaod.org>
> Reviewed-by: Eric Blake<eblake@redhat.com>
> [Comments merged properly with recent commit "error: Document Error
> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
> before its helpers, and touch up style. Commit message tweaked.]
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
Ok, I see you have mostly rewritten the big comment and not only in this patch, so, I go and read the whole comment on top of these series.
=================================
* Pass an existing error to the caller with the message modified:
* error_propagate_prepend(errp, err,
* "Could not frobnicate '%s': ", name);
* This is more concise than
* error_propagate(errp, err); // don't do this
* error_prepend(errp, "Could not frobnicate '%s': ", name);
* and works even when @errp is &error_fatal.
- the latter doesn't consider ERRP_AUTO_PROPAGATE: as we know, that ERRP_AUTO_PROPAGATE should be used when we use error_prepend, the latter should look like
ERRP_AUTO_PROPAGATE();
...
error_propagate(errp, err); // don't do this
error_prepend(errp, "Could not frobnicate '%s': ", name);
- and it works even when @errp is &error_fatal, so the error_propagate_prepend now is just a shortcut, not the only correct way.
Still, the text is formally correct as is, and may be improved later.
=================================
* 2. Replace &err by errp, and err by *errp. Delete local variable
* @err.
- hmm a bit not obvious,, It can be local_err. It can be (in some rare cases) still needed to handle the error locally, not passing to the caller..
may be just something like "Assume local Error *err variable is used to get errors from called functions and than propagated to caller's errp" before paragraph [2.] will help.
*
* 3. Delete error_propagate(errp, *errp), replace
* error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
*
* 4. Ensure @errp is valid at return: when you destroy *errp, set
* errp = NULL.
=================================
May be good to add note about ERRP_AUTO_PROPAGATE() into comment above error_append_hint (and error_(v)prepend)).
=================================
/*
* Make @errp parameter easier to use regardless of argument value
may be s/argument/its/
*
* This macro is for use right at the beginning of a function that
* takes an Error **errp parameter to pass errors to its caller. The
* parameter must be named @errp.
*
* It must be used when the function dereferences @errp or passes
* @errp to error_prepend(), error_vprepend(), or error_append_hint().
* It is safe to use even when it's not needed, but please avoid
* cluttering the source with useless code.
*
* If @errp is NULL or &error_fatal, rewrite it to point to a local
* Error variable, which will be automatically propagated to the
* original @errp on function exit.
*
* Note: &error_abort is not rewritten, because that would move the
* abort from the place where the error is created to the place where
* it's propagated.
*/
=================================
All these are minor, the documentation is good as is, thank you!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
2020-07-07 19:36 ` Eric Blake
@ 2020-07-07 20:11 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 20:11 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, vsementsov, qemu-block, Paul Durrant, Laszlo Ersek,
Christian Schoenebeck, qemu-devel, Michael Roth, groug,
Stefano Stabellini, Gerd Hoffmann, Stefan Hajnoczi,
Anthony Perard, xen-devel, Max Reitz, Philippe Mathieu-Daudé
Eric Blake <eblake@redhat.com> writes:
> On 7/7/20 11:50 AM, Markus Armbruster wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>> does corresponding changes in code (look for details in
>> include/qapi/error.h)
>>
>> Usage example:
>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>> --max-width 80 FILES...
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> scripts/coccinelle/auto-propagated-errp.cocci | 337 ++++++++++++++++++
>> include/qapi/error.h | 3 +
>> MAINTAINERS | 1 +
>> 3 files changed, 341 insertions(+)
>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>
> Needs a tweak if we go with ERRP_GUARD. But that's easy.
>
>> +
>> +// Convert special case with goto separately.
>> +// I tried merging this into the following rule the obvious way, but
>> +// it made Coccinelle hang on block.c
>> +//
>> +// Note interesting thing: if we don't do it here, and try to fixup
>> +// "out: }" things later after all transformations (the rule will be
>> +// the same, just without error_propagate() call), coccinelle fails to
>> +// match this "out: }".
>
> "out: }" is not valid C; would referring to "out: ; }" fare any better?
We can try for the next batch.
>> +@ disable optional_qualifier@
>> +identifier rule1.fn, rule1.local_err, out;
>> +symbol errp;
>> +@@
>> +
>> + fn(..., Error ** ____, ...)
>> + {
>> + <...
>> +- goto out;
>> ++ return;
>> + ...>
>> +- out:
>> +- error_propagate(errp, local_err);
>> + }
>> +
>> +// Convert most of local_err related stuff.
>> +//
>> +// Note, that we inherit rule1.fn and rule1.local_err names, not
>> +// objects themselves. We may match something not related to the
>> +// pattern matched by rule1. For example, local_err may be defined with
>> +// the same name in different blocks inside one function, and in one
>> +// block follow the propagation pattern and in other block doesn't.
>> +//
>> +// Note also that errp-cleaning functions
>> +// error_free_errp
>> +// error_report_errp
>> +// error_reportf_errp
>> +// warn_report_errp
>> +// warn_reportf_errp
>> +// are not yet implemented. They must call corresponding Error* -
>> +// freeing function and then set *errp to NULL, to avoid further
>> +// propagation to original errp (consider ERRP_AUTO_PROPAGATE in use).
>> +// For example, error_free_errp may look like this:
>> +//
>> +// void error_free_errp(Error **errp)
>> +// {
>> +// error_free(*errp);
>> +// *errp = NULL;
>> +// }
>
> I guess we can still decide later if we want these additional
> functions, or if they will even help after the number of places we
> have already improved after applying this script as-is and with
> Markus' cleanups in place.
Yes.
> While I won't call myself a Coccinelle expert, it at least looks sane
> enough that I'm comfortable if you add:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
2020-07-07 20:08 ` Vladimir Sementsov-Ogievskiy
(?)
@ 2020-07-07 20:43 ` Markus Armbruster
-1 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2020-07-07 20:43 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, Stefano Stabellini, qemu-block, Paul Durrant,
Philippe Mathieu-Daudé,
Christian Schoenebeck, qemu-devel, Michael Roth, groug,
Gerd Hoffmann, Stefan Hajnoczi, Anthony Perard, xen-devel,
Max Reitz, Laszlo Ersek
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 07.07.2020 19:50, Markus Armbruster wrote:
>> From: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>
>> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with an errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort and error_propagate: when we wrap
>> error_abort by local_err+error_propagate, the resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
>> the local_err+error_propagate pattern, which will definitely fix the
>> issue) [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, later patches will add invocations
>> of this macro at the start of functions with either use
>> error_prepend/error_append_hint (solving 1) or which use
>> local_err+error_propagate to check errors, switching those
>> functions to use *errp instead (solving 2 and 3).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> Reviewed-by: Paul Durrant<paul@xen.org>
>> Reviewed-by: Greg Kurz<groug@kaod.org>
>> Reviewed-by: Eric Blake<eblake@redhat.com>
>> [Comments merged properly with recent commit "error: Document Error
>> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
>> before its helpers, and touch up style. Commit message tweaked.]
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>
> Ok, I see you have mostly rewritten the big comment
Guilty as charged... I was happy with the contents you provided (and
grateful for it), but our parallel work caused some redundancy. I went
beyond a minimal merge to get a something that reads as one coherent
text.
> and not only in this patch, so, I go and read the whole comment on top of these series.
>
> =================================
>
> * Pass an existing error to the caller with the message modified:
> * error_propagate_prepend(errp, err,
> * "Could not frobnicate '%s': ", name);
> * This is more concise than
> * error_propagate(errp, err); // don't do this
> * error_prepend(errp, "Could not frobnicate '%s': ", name);
> * and works even when @errp is &error_fatal.
>
> - the latter doesn't consider ERRP_AUTO_PROPAGATE: as we know, that ERRP_AUTO_PROPAGATE should be used when we use error_prepend, the latter should look like
>
>
> ERRP_AUTO_PROPAGATE();
> ...
> error_propagate(errp, err); // don't do this
> error_prepend(errp, "Could not frobnicate '%s': ", name);
>
> - and it works even when @errp is &error_fatal, so the error_propagate_prepend now is just a shortcut, not the only correct way.
I can duplicate the advice from the paragraph preceding it, like this:
* This is rarely needed. When @err is a local variable, use of
* ERRP_GUARD() commonly results in more readable code.
* Where it is needed, it is more concise than
* error_propagate(errp, err); // don't do this
* error_prepend(errp, "Could not frobnicate '%s': ", name);
* and works even when @errp is &error_fatal.
> Still, the text is formally correct as is, and may be improved later.
>
> =================================
>
> * 2. Replace &err by errp, and err by *errp. Delete local variable
> * @err.
>
> - hmm a bit not obvious,, It can be local_err.
Yes, but I trust the reader can make that mental jump.
> It can be (in some rare cases) still needed to handle the error locally, not passing to the caller..
I didn't think of this.
What about
* To convert a function to use ERRP_GUARD(), assuming the local
* variable it propagates to @errp is called @err:
[...]
* 2. Replace &err by errp, and err by *errp. Delete local variable
* @err if it s now unused.
Nope, still no good, if we replace like that, @err *will* be unused, and
the locally handled error will leak to the caller.
No time left for wordsmithing; let's improve on top.
> may be just something like "Assume local Error *err variable is used to get errors from called functions and than propagated to caller's errp" before paragraph [2.] will help.
>
>
> *
> * 3. Delete error_propagate(errp, *errp), replace
> * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
> *
> * 4. Ensure @errp is valid at return: when you destroy *errp, set
> * errp = NULL.
>
> =================================
>
>
> May be good to add note about ERRP_AUTO_PROPAGATE() into comment above error_append_hint (and error_(v)prepend)).
Good point.
> =================================
>
> /*
> * Make @errp parameter easier to use regardless of argument value
>
> may be s/argument/its/
>
> *
> * This macro is for use right at the beginning of a function that
> * takes an Error **errp parameter to pass errors to its caller. The
> * parameter must be named @errp.
> *
> * It must be used when the function dereferences @errp or passes
> * @errp to error_prepend(), error_vprepend(), or error_append_hint().
> * It is safe to use even when it's not needed, but please avoid
> * cluttering the source with useless code.
> *
> * If @errp is NULL or &error_fatal, rewrite it to point to a local
> * Error variable, which will be automatically propagated to the
> * original @errp on function exit.
> *
> * Note: &error_abort is not rewritten, because that would move the
> * abort from the place where the error is created to the place where
> * it's propagated.
> */
>
> =================================
>
>
> All these are minor, the documentation is good as is, thank you!
Thanks for your review, and your patience!
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-07-07 20:44 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 16:50 [PATCH v12 0/8] error: auto propagated local_err part I Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE() Markus Armbruster
2020-07-07 16:50 ` Markus Armbruster
2020-07-07 19:02 ` Eric Blake
2020-07-07 20:01 ` Markus Armbruster
2020-07-07 20:08 ` Vladimir Sementsov-Ogievskiy
2020-07-07 20:08 ` Vladimir Sementsov-Ogievskiy
2020-07-07 20:43 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE() Markus Armbruster
2020-07-07 19:36 ` Eric Blake
2020-07-07 20:11 ` Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 3/8] sd: Use ERRP_AUTO_PROPAGATE() Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 4/8] pflash: " Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 5/8] fw_cfg: " Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 6/8] virtio-9p: " Markus Armbruster
2020-07-07 16:50 ` [PATCH v12 7/8] nbd: " Markus Armbruster
2020-07-07 19:43 ` Eric Blake
2020-07-07 16:50 ` [PATCH v12 8/8] xen: " Markus Armbruster
2020-07-07 18:47 ` [PATCH v12 0/8] error: auto propagated local_err part I Vladimir Sementsov-Ogievskiy
2020-07-07 18:52 ` Eric Blake
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.