yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone
a19a21
From 46c3298774b976cc6a1cd834751e644fb482b08e Mon Sep 17 00:00:00 2001
709dde
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com>
a19a21
Date: Wed, 16 Dec 2020 16:06:10 -0500
a19a21
Subject: [PATCH 09/14] error: New macro ERRP_GUARD()
709dde
MIME-Version: 1.0
709dde
Content-Type: text/plain; charset=UTF-8
709dde
Content-Transfer-Encoding: 8bit
709dde
709dde
RH-Author: Marc-André Lureau <marcandre.lureau@redhat.com>
a19a21
Message-id: <20201216160615.324213-6-marcandre.lureau@redhat.com>
a19a21
Patchwork-id: 100476
a19a21
O-Subject: [RHEL-8.4.0 qemu-kvm PATCH v2 05/10] error: New macro ERRP_GUARD()
a19a21
Bugzilla: 1859494
a19a21
RH-Acked-by: Danilo de Paula <ddepaula@redhat.com>
a19a21
RH-Acked-by: Sergio Lopez Pascual <slp@redhat.com>
a19a21
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
709dde
709dde
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
709dde
709dde
Introduce a new ERRP_GUARD() macro, to be used at start of functions
709dde
with an errp OUT parameter.
709dde
709dde
It has three goals:
709dde
709dde
1. Fix issue with error_fatal and error_prepend/error_append_hint: the
709dde
user can't see this additional information, because exit() happens in
709dde
error_setg earlier than information is added. [Reported by Greg Kurz]
709dde
709dde
2. Fix issue with error_abort and error_propagate: when we wrap
709dde
error_abort by local_err+error_propagate, the resulting coredump will
709dde
refer to error_propagate and not to the place where error happened.
709dde
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
709dde
the local_err+error_propagate pattern, which will definitely fix the
709dde
issue) [Reported by Kevin Wolf]
709dde
709dde
3. Drop local_err+error_propagate pattern, which is used to workaround
709dde
void functions with errp parameter, when caller wants to know resulting
709dde
status. (Note: actually these functions could be merely updated to
709dde
return int error code).
709dde
709dde
To achieve these goals, later patches will add invocations
709dde
of this macro at the start of functions with either use
709dde
error_prepend/error_append_hint (solving 1) or which use
709dde
local_err+error_propagate to check errors, switching those
709dde
functions to use *errp instead (solving 2 and 3).
709dde
709dde
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
709dde
Reviewed-by: Paul Durrant <paul@xen.org>
709dde
Reviewed-by: Greg Kurz <groug@kaod.org>
709dde
Reviewed-by: Eric Blake <eblake@redhat.com>
709dde
[Merge comments properly with recent commit "error: Document Error API
709dde
usage rules", and edit for clarity.  Put ERRP_AUTO_PROPAGATE() before
709dde
its helpers, and touch up style.  Tweak commit message.]
709dde
Signed-off-by: Markus Armbruster <armbru@redhat.com>
709dde
Message-Id: <20200707165037.1026246-2-armbru@redhat.com>
709dde
709dde
(cherry picked from commit ae7c80a7bd73685437bf6ba9d7c26098351f4166)
709dde
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
709dde
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
709dde
---
709dde
 include/qapi/error.h | 158 +++++++++++++++++++++++++++++++++++++------
709dde
 1 file changed, 139 insertions(+), 19 deletions(-)
709dde
709dde
diff --git a/include/qapi/error.h b/include/qapi/error.h
709dde
index 08d48e74836..e658790acfc 100644
709dde
--- a/include/qapi/error.h
709dde
+++ b/include/qapi/error.h
709dde
@@ -30,6 +30,10 @@
709dde
  *   job.  Since the value of @errp is about handling the error, the
709dde
  *   function should not examine it.
709dde
  *
709dde
+ * - The function may pass @errp to functions it calls to pass on
709dde
+ *   their errors to its caller.  If it dereferences @errp to check
709dde
+ *   for errors, it must use ERRP_GUARD().
709dde
+ *
709dde
  * - On success, the function should not touch *errp.  On failure, it
709dde
  *   should set a new error, e.g. with error_setg(errp, ...), or
709dde
  *   propagate an existing one, e.g. with error_propagate(errp, ...).
709dde
@@ -45,15 +49,17 @@
709dde
  * = Creating errors =
709dde
  *
709dde
  * Create an error:
709dde
- *     error_setg(&err, "situation normal, all fouled up");
709dde
+ *     error_setg(errp, "situation normal, all fouled up");
709dde
+ * where @errp points to the location to receive the error.
709dde
  *
709dde
  * Create an error and add additional explanation:
709dde
- *     error_setg(&err, "invalid quark");
709dde
- *     error_append_hint(&err, "Valid quarks are up, down, strange, "
709dde
+ *     error_setg(errp, "invalid quark");
709dde
+ *     error_append_hint(errp, "Valid quarks are up, down, strange, "
709dde
  *                       "charm, top, bottom.\n");
709dde
+ * This may require use of ERRP_GUARD(); more on that below.
709dde
  *
709dde
  * Do *not* contract this to
709dde
- *     error_setg(&err, "invalid quark\n" // WRONG!
709dde
+ *     error_setg(errp, "invalid quark\n" // WRONG!
709dde
  *                "Valid quarks are up, down, strange, charm, top, bottom.");
709dde
  *
709dde
  * = Reporting and destroying errors =
709dde
@@ -107,18 +113,6 @@
709dde
  * Errors get passed to the caller through the conventional @errp
709dde
  * parameter.
709dde
  *
709dde
- * Pass an existing error to the caller:
709dde
- *     error_propagate(errp, err);
709dde
- * where Error **errp is a parameter, by convention the last one.
709dde
- *
709dde
- * Pass an existing error to the caller with the message modified:
709dde
- *     error_propagate_prepend(errp, err,
709dde
- *                             "Could not frobnicate '%s': ", name);
709dde
- * This is more concise than
709dde
- *     error_propagate(errp, err); // don't do this
709dde
- *     error_prepend(errp, "Could not frobnicate '%s': ", name);
709dde
- * and works even when @errp is &error_fatal.
709dde
- *
709dde
  * Create a new error and pass it to the caller:
709dde
  *     error_setg(errp, "situation normal, all fouled up");
709dde
  *
709dde
@@ -129,18 +123,26 @@
709dde
  *         handle the error...
709dde
  *     }
709dde
  * - when it does not, say because it is a void function:
709dde
+ *     ERRP_GUARD();
709dde
+ *     foo(arg, errp);
709dde
+ *     if (*errp) {
709dde
+ *         handle the error...
709dde
+ *     }
709dde
+ * More on ERRP_GUARD() below.
709dde
+ *
709dde
+ * Code predating ERRP_GUARD() still exists, and looks like this:
709dde
  *     Error *err = NULL;
709dde
  *     foo(arg, &err;;
709dde
  *     if (err) {
709dde
  *         handle the error...
709dde
- *         error_propagate(errp, err);
709dde
+ *         error_propagate(errp, err); // deprecated
709dde
  *     }
709dde
- * Do *not* "optimize" this to
709dde
+ * Avoid in new code.  Do *not* "optimize" it to
709dde
  *     foo(arg, errp);
709dde
  *     if (*errp) { // WRONG!
709dde
  *         handle the error...
709dde
  *     }
709dde
- * because errp may be NULL!
709dde
+ * because errp may be NULL without the ERRP_GUARD() guard.
709dde
  *
709dde
  * But when all you do with the error is pass it on, please use
709dde
  *     foo(arg, errp);
709dde
@@ -160,6 +162,19 @@
709dde
  *         handle the error...
709dde
  *     }
709dde
  *
709dde
+ * Pass an existing error to the caller:
709dde
+ *     error_propagate(errp, err);
709dde
+ * This is rarely needed.  When @err is a local variable, use of
709dde
+ * ERRP_GUARD() commonly results in more readable code.
709dde
+ *
709dde
+ * Pass an existing error to the caller with the message modified:
709dde
+ *     error_propagate_prepend(errp, err,
709dde
+ *                             "Could not frobnicate '%s': ", name);
709dde
+ * This is more concise than
709dde
+ *     error_propagate(errp, err); // don't do this
709dde
+ *     error_prepend(errp, "Could not frobnicate '%s': ", name);
709dde
+ * and works even when @errp is &error_fatal.
709dde
+ *
709dde
  * Receive and accumulate multiple errors (first one wins):
709dde
  *     Error *err = NULL, *local_err = NULL;
709dde
  *     foo(arg, &err;;
709dde
@@ -187,6 +202,69 @@
709dde
  *         error_setg(&err, ...); // WRONG!
709dde
  *     }
709dde
  * because this may pass a non-null err to error_setg().
709dde
+ *
709dde
+ * = Why, when and how to use ERRP_GUARD() =
709dde
+ *
709dde
+ * Without ERRP_GUARD(), use of the @errp parameter is restricted:
709dde
+ * - It must not be dereferenced, because it may be null.
709dde
+ * - It should not be passed to error_prepend() or
709dde
+ *   error_append_hint(), because that doesn't work with &error_fatal.
709dde
+ * ERRP_GUARD() lifts these restrictions.
709dde
+ *
709dde
+ * To use ERRP_GUARD(), add it right at the beginning of the function.
709dde
+ * @errp can then be used without worrying about the argument being
709dde
+ * NULL or &error_fatal.
709dde
+ *
709dde
+ * Using it when it's not needed is safe, but please avoid cluttering
709dde
+ * the source with useless code.
709dde
+ *
709dde
+ * = Converting to ERRP_GUARD() =
709dde
+ *
709dde
+ * To convert a function to use ERRP_GUARD():
709dde
+ *
709dde
+ * 0. If the Error ** parameter is not named @errp, rename it to
709dde
+ *    @errp.
709dde
+ *
709dde
+ * 1. Add an ERRP_GUARD() invocation, by convention right at the
709dde
+ *    beginning of the function.  This makes @errp safe to use.
709dde
+ *
709dde
+ * 2. Replace &err by errp, and err by *errp.  Delete local variable
709dde
+ *    @err.
709dde
+ *
709dde
+ * 3. Delete error_propagate(errp, *errp), replace
709dde
+ *    error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...)
709dde
+ *
709dde
+ * 4. Ensure @errp is valid at return: when you destroy *errp, set
709dde
+ *    errp = NULL.
709dde
+ *
709dde
+ * Example:
709dde
+ *
709dde
+ *     bool fn(..., Error **errp)
709dde
+ *     {
709dde
+ *         Error *err = NULL;
709dde
+ *
709dde
+ *         foo(arg, &err;;
709dde
+ *         if (err) {
709dde
+ *             handle the error...
709dde
+ *             error_propagate(errp, err);
709dde
+ *             return false;
709dde
+ *         }
709dde
+ *         ...
709dde
+ *     }
709dde
+ *
709dde
+ * becomes
709dde
+ *
709dde
+ *     bool fn(..., Error **errp)
709dde
+ *     {
709dde
+ *         ERRP_GUARD();
709dde
+ *
709dde
+ *         foo(arg, errp);
709dde
+ *         if (*errp) {
709dde
+ *             handle the error...
709dde
+ *             return false;
709dde
+ *         }
709dde
+ *         ...
709dde
+ *     }
709dde
  */
709dde
 
709dde
 #ifndef ERROR_H
709dde
@@ -287,6 +365,7 @@ void error_setg_win32_internal(Error **errp,
709dde
  * the error object.
709dde
  * Else, move the error object from @local_err to *@dst_errp.
709dde
  * On return, @local_err is invalid.
709dde
+ * Please use ERRP_GUARD() instead when possible.
709dde
  * Please don't error_propagate(&error_fatal, ...), use
709dde
  * error_report_err() and exit(), because that's more obvious.
709dde
  */
709dde
@@ -298,6 +377,7 @@ void error_propagate(Error **dst_errp, Error *local_err);
709dde
  * Behaves like
709dde
  *     error_prepend(&local_err, fmt, ...);
709dde
  *     error_propagate(dst_errp, local_err);
709dde
+ * Please use ERRP_GUARD() and error_prepend() instead when possible.
709dde
  */
709dde
 void error_propagate_prepend(Error **dst_errp, Error *local_err,
709dde
                              const char *fmt, ...);
709dde
@@ -395,6 +475,46 @@ void error_set_internal(Error **errp,
709dde
                         ErrorClass err_class, const char *fmt, ...)
709dde
     GCC_FMT_ATTR(6, 7);
709dde
 
709dde
+/*
709dde
+ * Make @errp parameter easier to use regardless of argument value
709dde
+ *
709dde
+ * This macro is for use right at the beginning of a function that
709dde
+ * takes an Error **errp parameter to pass errors to its caller.  The
709dde
+ * parameter must be named @errp.
709dde
+ *
709dde
+ * It must be used when the function dereferences @errp or passes
709dde
+ * @errp to error_prepend(), error_vprepend(), or error_append_hint().
709dde
+ * It is safe to use even when it's not needed, but please avoid
709dde
+ * cluttering the source with useless code.
709dde
+ *
709dde
+ * If @errp is NULL or &error_fatal, rewrite it to point to a local
709dde
+ * Error variable, which will be automatically propagated to the
709dde
+ * original @errp on function exit.
709dde
+ *
709dde
+ * Note: &error_abort is not rewritten, because that would move the
709dde
+ * abort from the place where the error is created to the place where
709dde
+ * it's propagated.
709dde
+ */
709dde
+#define ERRP_GUARD()                                            \
709dde
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};   \
709dde
+    do {                                                        \
709dde
+        if (!errp || errp == &error_fatal) {                    \
709dde
+            errp = &_auto_errp_prop.local_err;                  \
709dde
+        }                                                       \
709dde
+    } while (0)
709dde
+
709dde
+typedef struct ErrorPropagator {
709dde
+    Error *local_err;
709dde
+    Error **errp;
709dde
+} ErrorPropagator;
709dde
+
709dde
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
709dde
+{
709dde
+    error_propagate(prop->errp, prop->local_err);
709dde
+}
709dde
+
709dde
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
709dde
+
709dde
 /*
709dde
  * Special error destination to abort on error.
709dde
  * See error_setg() and error_propagate() for details.
709dde
-- 
709dde
2.27.0
709dde