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

Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err


  • To: Markus Armbruster <armbru@xxxxxxxxxx>
  • From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
  • Date: Fri, 21 Feb 2020 12:42:06 +0300
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=virtuozzo.com; dmarc=pass action=none header.from=virtuozzo.com; dkim=pass header.d=virtuozzo.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ANrgGt89xnsTdD129Tx6yS+FCPRYqGtcR2MAwyoM29M=; b=WDgOQqULarZahFgFCOmYWf6Xnd8in6w4wRotkL2KDCrLSi20OmAI0dx+iTReRcxlJJq+VGn9FsJxKcsDl/ds/Y7/HrjW5twLvA/G52gvc/d4SIvdcJ52UVvYAoQB0cTU/eG8aAsuCdy85ONzUFiu7fj861RuW3W3jKGON05crjyb5REyGLHtEwECG7wSXMch/Njo0kjHYPfYbWNezONtZ9S23zth5a97N7wIDgw8wI/bZL1VtLECM4fJbKt4cKRp6TcS1blJETTcAes96gZuCE1dPqDoyuOpq9j7zoYwO1+fX/a1exPMKa6vdlMBpMSLSsFTK3sJ1bMsAHIitQGwag==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Vy8V1kTAtHzczIRvLEqOLHjId+WLYiFBc3WM3kQxW8c9JrM/0PmJjSGoLu15sugthx4k+2hu1ahtCpIV4NK7GzdWRgGsxxSidQzwZPqJsVSMI7BliHmW30utXs1bq24GaCHsW7oHR1oCiO+dONKRG1xAZwu9rx0s28pnFI4cgkRvFIvbcvkJHyZabM3VrncNskCepjzMn+aR2M9hRnjBwHn/0hFbqIrNhij9ypsR83r4ww24sFtiBi1G4+pKdNAHqbuPaMd5NnUsL8Jw82gS5C8HYgOSqEUtj7XDUAFMNWCWooAuzqbqCI13kKKaodtstq/A9UY+kCZ/3WntRIZE5w==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=vsementsov@xxxxxxxxxxxxx;
  • Cc: Kevin Wolf <kwolf@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>, qemu-block@xxxxxxxxxx, Paul Durrant <paul@xxxxxxx>, Laszlo Ersek <lersek@xxxxxxxxxx>, qemu-devel@xxxxxxxxxx, Max Reitz <mreitz@xxxxxxxxxx>, Greg Kurz <groug@xxxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>, Stefan Hajnoczi <stefanha@xxxxxxxxxx>, Anthony Perard <anthony.perard@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>, Stefan Berger <stefanb@xxxxxxxxxxxxx>
  • Delivery-date: Fri, 21 Feb 2020 09:42:37 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

21.02.2020 12:19, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:

Here is introduced 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@xxxxxxxxxxxxx>
Reviewed-by: Greg Kurz <groug@xxxxxxxx>
Reviewed-by: Eric Blake <eblake@xxxxxxxxxx>
---

CC: Eric Blake <eblake@xxxxxxxxxx>
CC: Kevin Wolf <kwolf@xxxxxxxxxx>
CC: Max Reitz <mreitz@xxxxxxxxxx>
CC: Greg Kurz <groug@xxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
CC: Paul Durrant <paul@xxxxxxx>
CC: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
CC: "Philippe Mathieu-Daudé" <philmd@xxxxxxxxxx>
CC: Laszlo Ersek <lersek@xxxxxxxxxx>
CC: Gerd Hoffmann <kraxel@xxxxxxxxxx>
CC: Stefan Berger <stefanb@xxxxxxxxxxxxx>
CC: Markus Armbruster <armbru@xxxxxxxxxx>
CC: Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>
CC: qemu-block@xxxxxxxxxx
CC: xen-devel@xxxxxxxxxxxxxxxxxxxx

  include/qapi/error.h | 83 +++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index d34987148d..b9452d4806 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -78,7 +78,7 @@
   * Call a function treating errors as fatal:
   *     foo(arg, &error_fatal);
   *
- * Receive an error and pass it on to the caller:
+ * Receive an error and pass it on to the caller (DEPRECATED*):
   *     Error *err = NULL;
   *     foo(arg, &err);
   *     if (err) {
@@ -98,6 +98,50 @@
   *     foo(arg, errp);
   * for readability.
   *
+ * DEPRECATED* This pattern is deprecated now, the use ERRP_AUTO_PROPAGATE 
macro
+ * instead (defined below).
+ * It's deprecated because of two things:
+ *
+ * 1. 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.
+ *
+ * 2. A lot of extra code of the same pattern
+ *
+ * How to update old code to use ERRP_AUTO_PROPAGATE?
+ *
+ * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
+ * than you may safely dereference errp to check errors and do not need any
+ * additional local Error variables or calls to error_propagate().
+ *
+ * Example:
+ *
+ * old code
+ *
+ *     void fn(..., Error **errp) {
+ *         Error *err = NULL;
+ *         foo(arg, &err);
+ *         if (err) {
+ *             handle the error...
+ *             error_propagate(errp, err);
+ *             return;
+ *         }
+ *         ...
+ *     }
+ *
+ * updated code
+ *
+ *     void fn(..., Error **errp) {
+ *         ERRP_AUTO_PROPAGATE();
+ *         foo(arg, errp);
+ *         if (*errp) {
+ *             handle the error...
+ *             return;
+ *         }
+ *         ...
+ *     }
+ *
+ *
   * Receive and accumulate multiple errors (first one wins):
   *     Error *err = NULL, *local_err = NULL;
   *     foo(arg, &err);

Let's explain what should be done *first*, and only then talk about the
deprecated pattern and how to convert it to current usage.

@@ -348,6 +392,43 @@ void error_set_internal(Error **errp,
                          ErrorClass err_class, const char *fmt, ...)
      GCC_FMT_ATTR(6, 7);
+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);
+
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro is created to be the first line of a function which use
+ * Error **errp parameter to report error. It's needed only in cases where we
+ * want to use error_prepend, error_append_hint or dereference *errp. It's
+ * still safe (but useless) in other cases.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information by error_prepend or
+ * error_append_hint (as, if it was error_fatal, we swapped it with a
+ * local_error to be propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.

Tradeoff: we gain more useful backtraces, we lose message improvements
from error_prepend(), error_append_hint() and such, if any.  Makes
sense.

+ */

The comment's contents looks okay to me.  I'll want to tweak formatting
to better blend in with the rest of this file, but let's not worry about
that now.

+#define ERRP_AUTO_PROPAGATE()                                  \
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
+    errp = ((errp == NULL || *errp == error_fatal)             \
+            ? &_auto_errp_prop.local_err : errp)
+
  /*
   * Special error destination to abort on error.
   * See error_setg() and error_propagate() for details.

*errp == error_fatal tests *errp == NULL, which is not what you want.
You need to test errp == &error_fatal, just like error_handle_fatal().

Oops, great bug) And nobody noticed before) Of course, you are right.


Superfluous parenthesis around the first operand of ?:.

Wouldn't

    #define ERRP_AUTO_PROPAGATE()                                  \
        g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
        if (!errp || errp == &error_fatal) {                       \
            errp = &_auto_errp_prop.local_err;                     \
        }

be clearer?


Hmm, notation with "if" will allow omitting ';' after macro invocation, which 
seems not good..
And if I'm not wrong we've already discussed it somewhere in previous versions.

Still, no objections for s/errp == NULL/!errp/ and we need s/*errp == 
error_fatal/errp == &error_fatal/ for sure.

--
Best regards,
Vladimir

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.