[Qemu-devel] [PULL 0/7] error: On abort, report where the error was created

[Qemu-devel] [PULL 0/7] error: On abort, report where the error was created

[Markus Armbruster]

The following changes since commit fc04a730b7e60f4a62d6260d4eb9c537d1d3643f:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20150908' into staging (2015-09-08 18:02:36 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-error-2015-09-10

for you to fetch changes up to 1e9b65bb1bad51735cab6c861c29b592dccabf0e:

  error: On abort, report where the error was created (2015-09-10 13:48:06 +0200)

----------------------------------------------------------------
error: On abort, report where the error was created

----------------------------------------------------------------
Markus Armbruster (7):
      error: De-duplicate code creating Error objects
      error: Make error_setg() a function
      qga: Clean up unnecessarily dirty casts
      qga/vss-win32: Document the DLL requires non-null errp
      error: error_set_errno() is unused, drop
      error: Revamp interface documentation
      error: On abort, report where the error was created

 include/qapi/error.h        | 234 +++++++++++++++++++++++++++++++-------------
 qga/vss-win32.c             |   6 +-
 qga/vss-win32/requester.cpp |   8 +-
 qga/vss-win32/requester.h   |  13 ++-
 util/error.c                | 120 +++++++++++++----------
 5 files changed, 246 insertions(+), 135 deletions(-)

-- 
2.4.3

[Qemu-devel] [PULL 1/7] error: De-duplicate code creating Error objects

[Markus Armbruster]

Duplicated when commit 680d16d added error_set_errno(), and again when
commit 20840d4 added error_set_win32().

Make the original copy in error_set() reusable by factoring out
error_setv(), then rewrite error_set_errno() and error_set_win32() on
top of it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 util/error.c | 70 ++++++++++++++++++++++--------------------------------------
 1 file changed, 26 insertions(+), 44 deletions(-)

diff --git a/util/error.c b/util/error.c
index 14f4351..4518642 100644
--- a/util/error.c
+++ b/util/error.c
@@ -22,10 +22,10 @@ struct Error
 
 Error *error_abort;
 
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+static void error_setv(Error **errp, ErrorClass err_class,
+                       const char *fmt, va_list ap)
 {
     Error *err;
-    va_list ap;
     int saved_errno = errno;
 
     if (errp == NULL) {
@@ -34,10 +34,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     assert(*errp == NULL);
 
     err = g_malloc0(sizeof(*err));
-
-    va_start(ap, fmt);
     err->msg = g_strdup_vprintf(fmt, ap);
-    va_end(ap);
     err->err_class = err_class;
 
     if (errp == &error_abort) {
@@ -50,38 +47,35 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     errno = saved_errno;
 }
 
+void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_setv(errp, err_class, fmt, ap);
+    va_end(ap);
+}
+
 void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                      const char *fmt, ...)
 {
-    Error *err;
-    char *msg1;
     va_list ap;
+    char *msg;
     int saved_errno = errno;
 
     if (errp == NULL) {
         return;
     }
-    assert(*errp == NULL);
-
-    err = g_malloc0(sizeof(*err));
 
     va_start(ap, fmt);
-    msg1 = g_strdup_vprintf(fmt, ap);
+    error_setv(errp, err_class, fmt, ap);
+    va_end(ap);
+
     if (os_errno != 0) {
-        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
-        g_free(msg1);
-    } else {
-        err->msg = msg1;
+        msg = (*errp)->msg;
+        (*errp)->msg = g_strdup_printf("%s: %s", msg, strerror(os_errno));
+        g_free(msg);
     }
-    va_end(ap);
-    err->err_class = err_class;
-
-    if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
-    }
-
-    *errp = err;
 
     errno = saved_errno;
 }
@@ -96,37 +90,25 @@ void error_setg_file_open(Error **errp, int os_errno, const char *filename)
 void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
                      const char *fmt, ...)
 {
-    Error *err;
-    char *msg1;
     va_list ap;
+    char *msg1, *msg2;
 
     if (errp == NULL) {
         return;
     }
-    assert(*errp == NULL);
-
-    err = g_malloc0(sizeof(*err));
 
     va_start(ap, fmt);
-    msg1 = g_strdup_vprintf(fmt, ap);
+    error_setv(errp, err_class, fmt, ap);
+    va_end(ap);
+
     if (win32_err != 0) {
-        char *msg2 = g_win32_error_message(win32_err);
-        err->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
-                                   (unsigned)win32_err);
+        msg1 = (*errp)->msg;
+        msg2 = g_win32_error_message(win32_err);
+        (*errp)->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
+                                       (unsigned)win32_err);
         g_free(msg2);
         g_free(msg1);
-    } else {
-        err->msg = msg1;
     }
-    va_end(ap);
-    err->err_class = err_class;
-
-    if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
-    }
-
-    *errp = err;
 }
 
 #endif
-- 
2.4.3

[Qemu-devel] [PULL 2/7] error: Make error_setg() a function

[Markus Armbruster]

Saves a tiny amount of code at every call site.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/error.h | 4 ++--
 util/error.c         | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index f44c451..34af4e1 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -51,8 +51,8 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
 /**
  * Same as error_set(), but sets a generic error
  */
-#define error_setg(errp, fmt, ...) \
-    error_set(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+void error_setg(Error **errp, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
 #define error_setg_errno(errp, os_error, fmt, ...) \
     error_set_errno(errp, os_error, ERROR_CLASS_GENERIC_ERROR, \
                     fmt, ## __VA_ARGS__)
diff --git a/util/error.c b/util/error.c
index 4518642..8f12f67 100644
--- a/util/error.c
+++ b/util/error.c
@@ -56,6 +56,15 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     va_end(ap);
 }
 
+void error_setg(Error **errp, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    va_end(ap);
+}
+
 void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                      const char *fmt, ...)
 {
-- 
2.4.3

[Qemu-devel] [PULL 3/7] qga: Clean up unnecessarily dirty casts

[Markus Armbruster]

qga_vss_fsfreeze() casts error_set_win32() from

    void (*)(Error **, int, ErrorClass, const char *, ...)

to

    void (*)(void **, int, int, const char *, ...)

The result is later called.  Since the two types are not compatible,
the call is undefined behavior.  It works in practice anyway.

However, there's no real need for trickery here.  Clean it up as
follows:

* Declare struct Error, and fix the first parameter.

* Switch to error_setg_win32().  This gets rid of the troublesome
  ErrorClass parameter.  Requires converting error_setg_win32() from
  macro to function, but that's trivially easy, because this is the
  only user of error_set_win32().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/error.h        |  9 ++-------
 qga/vss-win32.c             |  5 ++---
 qga/vss-win32/requester.cpp |  2 +-
 qga/vss-win32/requester.h   | 11 ++++++-----
 util/error.c                |  5 ++---
 5 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 34af4e1..692e013 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -44,8 +44,8 @@ void error_set_errno(Error **errp, int os_error, ErrorClass err_class,
  * printf-style human message, followed by a g_win32_error_message() string if
  * @win32_err is not zero.
  */
-void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
-                     const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(3, 4);
 #endif
 
 /**
@@ -56,11 +56,6 @@ void error_setg(Error **errp, const char *fmt, ...)
 #define error_setg_errno(errp, os_error, fmt, ...) \
     error_set_errno(errp, os_error, ERROR_CLASS_GENERIC_ERROR, \
                     fmt, ## __VA_ARGS__)
-#ifdef _WIN32
-#define error_setg_win32(errp, win32_err, fmt, ...) \
-    error_set_win32(errp, win32_err, ERROR_CLASS_GENERIC_ERROR, \
-                    fmt, ## __VA_ARGS__)
-#endif
 
 /**
  * Helper for open() errors
diff --git a/qga/vss-win32.c b/qga/vss-win32.c
index 0e40957..e1f5398 100644
--- a/qga/vss-win32.c
+++ b/qga/vss-win32.c
@@ -150,9 +150,8 @@ void qga_vss_fsfreeze(int *nr_volume, Error **errp, bool freeze)
     const char *func_name = freeze ? "requester_freeze" : "requester_thaw";
     QGAVSSRequesterFunc func;
     ErrorSet errset = {
-        .error_set = (ErrorSetFunc)error_set_win32,
-        .errp = (void **)errp,
-        .err_class = ERROR_CLASS_GENERIC_ERROR
+        .error_setg_win32 = error_setg_win32,
+        .errp = errp,
     };
 
     func = (QGAVSSRequesterFunc)GetProcAddress(provider_lib, func_name);
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 922e74d..b130fee 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -24,7 +24,7 @@
 #define VSS_TIMEOUT_EVENT_MSEC 10
 
 #define err_set(e, err, fmt, ...) \
-    ((e)->error_set((e)->errp, err, (e)->err_class, fmt, ## __VA_ARGS__))
+    ((e)->error_setg_win32((e)->errp, err, fmt, ## __VA_ARGS__))
 #define err_is_set(e) ((e)->errp && *(e)->errp)
 
 
diff --git a/qga/vss-win32/requester.h b/qga/vss-win32/requester.h
index 374f9b8..0a8d048 100644
--- a/qga/vss-win32/requester.h
+++ b/qga/vss-win32/requester.h
@@ -20,13 +20,14 @@
 extern "C" {
 #endif
 
+struct Error;
+
 /* Callback to set Error; used to avoid linking glib to the DLL */
-typedef void (*ErrorSetFunc)(void **errp, int win32_err, int err_class,
-                             const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+typedef void (*ErrorSetFunc)(struct Error **errp, int win32_err,
+                             const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 typedef struct ErrorSet {
-    ErrorSetFunc error_set;
-    void **errp;
-    int err_class;
+    ErrorSetFunc error_setg_win32;
+    struct Error **errp;
 } ErrorSet;
 
 STDAPI requester_init(void);
diff --git a/util/error.c b/util/error.c
index 8f12f67..9620f2a 100644
--- a/util/error.c
+++ b/util/error.c
@@ -96,8 +96,7 @@ void error_setg_file_open(Error **errp, int os_errno, const char *filename)
 
 #ifdef _WIN32
 
-void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
-                     const char *fmt, ...)
+void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
 {
     va_list ap;
     char *msg1, *msg2;
@@ -107,7 +106,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
     }
 
     va_start(ap, fmt);
-    error_setv(errp, err_class, fmt, ap);
+    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 
     if (win32_err != 0) {
-- 
2.4.3

[Qemu-devel] [PULL 4/7] qga/vss-win32: Document the DLL requires non-null errp

[Markus Armbruster]

requester.cpp uses this pattern to receive an error and pass it on to
the caller (err_is_set() macro peeled off for clarity):

    ... code that may set errset->errp ...
    if (errset->errp && *errset->errp) {
        ... handle error ...
    }

This breaks when errset->errp is null.  As far as I can tell, it
currently isn't, so this is merely fragile, not actually broken.

The robust way to do this is to receive the error in a local variable,
then propagate it up, like this:

    Error *err = NULL;

    ... code that may set err ...
    if (err)
        ... handle error ...
        error_propagate(errset->errp, err);
    }

See also commit 5e54769, 0f230bf, a903f40.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qga/vss-win32.c             | 1 +
 qga/vss-win32/requester.cpp | 3 ++-
 qga/vss-win32/requester.h   | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/vss-win32.c b/qga/vss-win32.c
index e1f5398..d75d7bb 100644
--- a/qga/vss-win32.c
+++ b/qga/vss-win32.c
@@ -154,6 +154,7 @@ void qga_vss_fsfreeze(int *nr_volume, Error **errp, bool freeze)
         .errp = errp,
     };
 
+    g_assert(errp);             /* requester.cpp requires it */
     func = (QGAVSSRequesterFunc)GetProcAddress(provider_lib, func_name);
     if (!func) {
         error_setg_win32(errp, GetLastError(), "failed to load %s from %s",
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index b130fee..aae0d5f 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -25,8 +25,9 @@
 
 #define err_set(e, err, fmt, ...) \
     ((e)->error_setg_win32((e)->errp, err, fmt, ## __VA_ARGS__))
+/* Bad idea, works only when (e)->errp != NULL: */
 #define err_is_set(e) ((e)->errp && *(e)->errp)
-
+/* To lift this restriction, error_propagate(), like we do in QEMU code */
 
 /* Handle to VSSAPI.DLL */
 static HMODULE hLib;
diff --git a/qga/vss-win32/requester.h b/qga/vss-win32/requester.h
index 0a8d048..34be5c1 100644
--- a/qga/vss-win32/requester.h
+++ b/qga/vss-win32/requester.h
@@ -27,7 +27,7 @@ typedef void (*ErrorSetFunc)(struct Error **errp, int win32_err,
                              const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 typedef struct ErrorSet {
     ErrorSetFunc error_setg_win32;
-    struct Error **errp;
+    struct Error **errp;        /* restriction: must not be null */
 } ErrorSet;
 
 STDAPI requester_init(void);
-- 
2.4.3

[Qemu-devel] [PULL 5/7] error: error_set_errno() is unused, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/error.h | 7 ++-----
 util/error.c         | 5 ++---
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 692e013..8c3a7dd 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -35,8 +35,8 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
  * printf-style human message, followed by a strerror() string if
  * @os_error is not zero.
  */
-void error_set_errno(Error **errp, int os_error, ErrorClass err_class,
-                     const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+void error_setg_errno(Error **errp, int os_error, const char *fmt, ...)
+    GCC_FMT_ATTR(3, 4);
 
 #ifdef _WIN32
 /**
@@ -53,9 +53,6 @@ void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
  */
 void error_setg(Error **errp, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
-#define error_setg_errno(errp, os_error, fmt, ...) \
-    error_set_errno(errp, os_error, ERROR_CLASS_GENERIC_ERROR, \
-                    fmt, ## __VA_ARGS__)
 
 /**
  * Helper for open() errors
diff --git a/util/error.c b/util/error.c
index 9620f2a..b842f20 100644
--- a/util/error.c
+++ b/util/error.c
@@ -65,8 +65,7 @@ void error_setg(Error **errp, const char *fmt, ...)
     va_end(ap);
 }
 
-void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
-                     const char *fmt, ...)
+void error_setg_errno(Error **errp, int os_errno, const char *fmt, ...)
 {
     va_list ap;
     char *msg;
@@ -77,7 +76,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     }
 
     va_start(ap, fmt);
-    error_setv(errp, err_class, fmt, ap);
+    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 
     if (os_errno != 0) {
-- 
2.4.3

[Qemu-devel] [PULL 6/7] error: Revamp interface documentation

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/error.h | 177 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 127 insertions(+), 50 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 8c3a7dd..358a9d3 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -2,13 +2,75 @@
  * QEMU Error Objects
  *
  * Copyright IBM, Corp. 2011
+ * Copyright (C) 2011-2015 Red Hat, Inc.
  *
  * Authors:
  *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Markus Armbruster <armbru@redhat.com>
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.  See
  * the COPYING.LIB file in the top-level directory.
  */
+
+/*
+ * Error reporting system loosely patterned after Glib's GError.
+ *
+ * Create an error:
+ *     error_setg(&err, "situation normal, all fouled up");
+ *
+ * Report an error to stderr:
+ *     error_report_err(err);
+ * This frees the error object.
+ *
+ * Report an error somewhere else:
+ *     const char *msg = error_get_pretty(err);
+ *     do with msg what needs to be done...
+ *     error_free(err);
+ *
+ * Handle an error without reporting it (just for completeness):
+ *     error_free(err);
+ *
+ * Pass an existing error to the caller:
+ *     error_propagate(errp, err);
+ * where Error **errp is a parameter, by convention the last one.
+ *
+ * Create a new error and pass it to the caller:
+ *     error_setg(errp, "situation normal, all fouled up");
+ *
+ * Call a function and receive an error from it:
+ *     Error *err = NULL;
+ *     foo(arg, &err);
+ *     if (err) {
+ *         handle the error...
+ *     }
+ *
+ * Call a function ignoring errors:
+ *     foo(arg, NULL);
+ *
+ * Call a function aborting on errors:
+ *     foo(arg, &error_abort);
+ *
+ * Receive an error and pass it on to the caller:
+ *     Error *err = NULL;
+ *     foo(arg, &err);
+ *     if (err) {
+ *         handle the error...
+ *         error_propagate(errp, err);
+ *     }
+ * where Error **errp is a parameter, by convention the last one.
+ *
+ * Do *not* "optimize" this to
+ *     foo(arg, errp);
+ *     if (*errp) { // WRONG!
+ *         handle the error...
+ *     }
+ * because errp may be NULL!
+ *
+ * But when all you do with the error is pass it on, please use
+ *     foo(arg, errp);
+ * for readability.
+ */
+
 #ifndef ERROR_H
 #define ERROR_H
 
@@ -16,85 +78,100 @@
 #include "qapi-types.h"
 #include <stdbool.h>
 
-/**
- * A class representing internal errors within QEMU.  An error has a ErrorClass
- * code and a human message.
+/*
+ * Opaque error object.
  */
 typedef struct Error Error;
 
-/**
- * Set an indirect pointer to an error given a ErrorClass value and a
- * printf-style human message.  This function is not meant to be used outside
- * of QEMU.
+/*
+ * Get @err's human-readable error message.
  */
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
-    GCC_FMT_ATTR(3, 4);
+const char *error_get_pretty(Error *err);
 
-/**
- * Set an indirect pointer to an error given a ErrorClass value and a
- * printf-style human message, followed by a strerror() string if
- * @os_error is not zero.
+/*
+ * Get @err's error class.
+ * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
+ * strongly discouraged.
+ */
+ErrorClass error_get_class(const Error *err);
+
+/*
+ * Create a new error object and assign it to *@errp.
+ * If @errp is NULL, the error is ignored.  Don't bother creating one
+ * then.
+ * If @errp is &error_abort, print a suitable message and abort().
+ * If @errp is anything else, *@errp must be NULL.
+ * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
+ * human-readable error message is made from printf-style @fmt, ...
+ */
+void error_setg(Error **errp, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
+/*
+ * Just like error_setg(), with @os_error info added to the message.
+ * If @os_error is non-zero, ": " + strerror(os_error) is appended to
+ * the human-readable error message.
  */
 void error_setg_errno(Error **errp, int os_error, const char *fmt, ...)
     GCC_FMT_ATTR(3, 4);
 
 #ifdef _WIN32
-/**
- * Set an indirect pointer to an error given a ErrorClass value and a
- * printf-style human message, followed by a g_win32_error_message() string if
- * @win32_err is not zero.
+/*
+ * Just like error_setg(), with @win32_error info added to the message.
+ * If @win32_error is non-zero, ": " + g_win32_error_message(win32_err)
+ * is appended to the human-readable error message.
  */
 void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
     GCC_FMT_ATTR(3, 4);
 #endif
 
-/**
- * Same as error_set(), but sets a generic error
+/*
+ * Propagate error object (if any) from @local_err to @dst_errp.
+ * If @local_err is NULL, do nothing (because there's nothing to
+ * propagate).
+ * Else, if @dst_errp is NULL, errors are being ignored.  Free the
+ * error object.
+ * Else, if @dst_errp is &error_abort, print a suitable message and
+ * abort().
+ * Else, if @dst_errp already contains an error, ignore this one: free
+ * the error object.
+ * Else, move the error object from @local_err to *@dst_errp.
+ * On return, @local_err is invalid.
  */
-void error_setg(Error **errp, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
+void error_propagate(Error **dst_errp, Error *local_err);
 
-/**
- * Helper for open() errors
+/*
+ * Convenience function to report open() failure.
  */
 void error_setg_file_open(Error **errp, int os_errno, const char *filename);
 
 /*
- * Get the error class of an error object.
- */
-ErrorClass error_get_class(const Error *err);
-
-/**
- * Returns an exact copy of the error passed as an argument.
+ * Return an exact copy of @err.
  */
 Error *error_copy(const Error *err);
 
-/**
- * Get a human readable representation of an error object.
- */
-const char *error_get_pretty(Error *err);
-
-/**
- * Convenience function to error_report() and free an error object.
- */
-void error_report_err(Error *);
-
-/**
- * Propagate an error to an indirect pointer to an error.  This function will
- * always transfer ownership of the error reference and handles the case where
- * dst_err is NULL correctly.  Errors after the first are discarded.
- */
-void error_propagate(Error **dst_errp, Error *local_err);
-
-/**
- * Free an error object.
+/*
+ * Free @err.
+ * @err may be NULL.
  */
 void error_free(Error *err);
 
-/**
- * If passed to error_set and friends, abort().
+/*
+ * Convenience function to error_report() and free @err.
  */
+void error_report_err(Error *);
 
+/*
+ * Just like error_setg(), except you get to specify the error class.
+ * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
+ * strongly discouraged.
+ */
+void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+    GCC_FMT_ATTR(3, 4);
+
+/*
+ * Pass to error_setg() & friends to abort() on error.
+ */
 extern Error *error_abort;
 
 #endif
-- 
2.4.3

[Qemu-devel] [PULL 7/7] error: On abort, report where the error was created

[Markus Armbruster]

This is particularly useful when we abort in error_propagate(),
because there the stack backtrace doesn't lead to where the error was
created.  Looks like this:

    Unexpected error in parse_block_error_action() at .../qemu/blockdev.c:322:
    qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid write error action
    Aborted (core dumped)

Note: to get this example output, I monkey-patched drive_new() to pass
&error_abort to blockdev_init().

To keep the error handling boiler plate from growing even more, all
error_setFOO() become macros expanding into error_setFOO_internal()
with additional __FILE__, __LINE__, __func__ arguments.  Not exactly
pretty, but it works.

The macro trickery breaks down when you take the address of an
error_setFOO().  Fortunately, we do that in just one place: qemu-ga's
Windows VSS provider and requester DLL wants to call
error_setg_win32() through a function pointer "to avoid linking glib
to the DLL".  Use error_setg_win32_internal() there.  The use of the
function pointer is already wrapped in a macro, so the churn isn't
bad.

Code size increases by some 35KiB for me (0.7%).  Tolerable.  Could be
less if we passed relative rather than absolute source file names to
the compiler, or forwent reporting __func__.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 include/qapi/error.h        | 43 +++++++++++++++++++++++++++--------
 qga/vss-win32.c             |  2 +-
 qga/vss-win32/requester.cpp |  5 +++--
 qga/vss-win32/requester.h   |  6 +++--
 util/error.c                | 55 ++++++++++++++++++++++++++++++++-------------
 5 files changed, 81 insertions(+), 30 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 358a9d3..426d5ea 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -104,16 +104,26 @@ ErrorClass error_get_class(const Error *err);
  * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
  * human-readable error message is made from printf-style @fmt, ...
  */
-void error_setg(Error **errp, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
+#define error_setg(errp, fmt, ...)                              \
+    error_setg_internal((errp), __FILE__, __LINE__, __func__,   \
+                        (fmt), ## __VA_ARGS__)
+void error_setg_internal(Error **errp,
+                         const char *src, int line, const char *func,
+                         const char *fmt, ...)
+    GCC_FMT_ATTR(5, 6);
 
 /*
  * Just like error_setg(), with @os_error info added to the message.
  * If @os_error is non-zero, ": " + strerror(os_error) is appended to
  * the human-readable error message.
  */
-void error_setg_errno(Error **errp, int os_error, const char *fmt, ...)
-    GCC_FMT_ATTR(3, 4);
+#define error_setg_errno(errp, os_error, fmt, ...)                      \
+    error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
+                              (os_error), (fmt), ## __VA_ARGS__)
+void error_setg_errno_internal(Error **errp,
+                               const char *fname, int line, const char *func,
+                               int os_error, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 
 #ifdef _WIN32
 /*
@@ -121,8 +131,13 @@ void error_setg_errno(Error **errp, int os_error, const char *fmt, ...)
  * If @win32_error is non-zero, ": " + g_win32_error_message(win32_err)
  * is appended to the human-readable error message.
  */
-void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
-    GCC_FMT_ATTR(3, 4);
+#define error_setg_win32(errp, win32_err, fmt, ...)                     \
+    error_setg_win32_internal((errp), __FILE__, __LINE__, __func__,     \
+                              (win32_err), (fmt), ## __VA_ARGS__)
+void error_setg_win32_internal(Error **errp,
+                               const char *src, int line, const char *func,
+                               int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 #endif
 
 /*
@@ -143,7 +158,12 @@ void error_propagate(Error **dst_errp, Error *local_err);
 /*
  * Convenience function to report open() failure.
  */
-void error_setg_file_open(Error **errp, int os_errno, const char *filename);
+#define error_setg_file_open(errp, os_errno, filename)                  \
+    error_setg_file_open_internal((errp), __FILE__, __LINE__, __func__, \
+                                  (os_errno), (filename))
+void error_setg_file_open_internal(Error **errp,
+                                   const char *src, int line, const char *func,
+                                   int os_errno, const char *filename);
 
 /*
  * Return an exact copy of @err.
@@ -166,8 +186,13 @@ void error_report_err(Error *);
  * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
  * strongly discouraged.
  */
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
-    GCC_FMT_ATTR(3, 4);
+#define error_set(errp, err_class, fmt, ...)                    \
+    error_set_internal((errp), __FILE__, __LINE__, __func__,    \
+                       (err_class), (fmt), ## __VA_ARGS__)
+void error_set_internal(Error **errp,
+                        const char *src, int line, const char *func,
+                        ErrorClass err_class, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 
 /*
  * Pass to error_setg() & friends to abort() on error.
diff --git a/qga/vss-win32.c b/qga/vss-win32.c
index d75d7bb..2142b49 100644
--- a/qga/vss-win32.c
+++ b/qga/vss-win32.c
@@ -150,7 +150,7 @@ void qga_vss_fsfreeze(int *nr_volume, Error **errp, bool freeze)
     const char *func_name = freeze ? "requester_freeze" : "requester_thaw";
     QGAVSSRequesterFunc func;
     ErrorSet errset = {
-        .error_setg_win32 = error_setg_win32,
+        .error_setg_win32 = error_setg_win32_internal,
         .errp = errp,
     };
 
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index aae0d5f..9b3e310 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -23,8 +23,9 @@
 /* Call QueryStatus every 10 ms while waiting for frozen event */
 #define VSS_TIMEOUT_EVENT_MSEC 10
 
-#define err_set(e, err, fmt, ...) \
-    ((e)->error_setg_win32((e)->errp, err, fmt, ## __VA_ARGS__))
+#define err_set(e, err, fmt, ...)                                       \
+    ((e)->error_setg_win32((e)->errp, __FILE__, __LINE__, __func__,     \
+                           err, fmt, ## __VA_ARGS__))
 /* Bad idea, works only when (e)->errp != NULL: */
 #define err_is_set(e) ((e)->errp && *(e)->errp)
 /* To lift this restriction, error_propagate(), like we do in QEMU code */
diff --git a/qga/vss-win32/requester.h b/qga/vss-win32/requester.h
index 34be5c1..c3093cf 100644
--- a/qga/vss-win32/requester.h
+++ b/qga/vss-win32/requester.h
@@ -23,8 +23,10 @@ extern "C" {
 struct Error;
 
 /* Callback to set Error; used to avoid linking glib to the DLL */
-typedef void (*ErrorSetFunc)(struct Error **errp, int win32_err,
-                             const char *fmt, ...) GCC_FMT_ATTR(3, 4);
+typedef void (*ErrorSetFunc)(struct Error **errp,
+                             const char *src, int line, const char *func,
+                             int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 typedef struct ErrorSet {
     ErrorSetFunc error_setg_win32;
     struct Error **errp;        /* restriction: must not be null */
diff --git a/util/error.c b/util/error.c
index b842f20..cdb726c 100644
--- a/util/error.c
+++ b/util/error.c
@@ -18,12 +18,23 @@ struct Error
 {
     char *msg;
     ErrorClass err_class;
+    const char *src, *func;
+    int line;
 };
 
 Error *error_abort;
 
-static void error_setv(Error **errp, ErrorClass err_class,
-                       const char *fmt, va_list ap)
+static void error_do_abort(Error *err)
+{
+    fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
+            err->func, err->src, err->line);
+    error_report_err(err);
+    abort();
+}
+
+static void error_setv(Error **errp,
+                       const char *src, int line, const char *func,
+                       ErrorClass err_class, const char *fmt, va_list ap)
 {
     Error *err;
     int saved_errno = errno;
@@ -36,10 +47,12 @@ static void error_setv(Error **errp, ErrorClass err_class,
     err = g_malloc0(sizeof(*err));
     err->msg = g_strdup_vprintf(fmt, ap);
     err->err_class = err_class;
+    err->src = src;
+    err->line = line;
+    err->func = func;
 
     if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
+        error_do_abort(err);
     }
 
     *errp = err;
@@ -47,25 +60,31 @@ static void error_setv(Error **errp, ErrorClass err_class,
     errno = saved_errno;
 }
 
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+void error_set_internal(Error **errp,
+                        const char *src, int line, const char *func,
+                        ErrorClass err_class, const char *fmt, ...)
 {
     va_list ap;
 
     va_start(ap, fmt);
-    error_setv(errp, err_class, fmt, ap);
+    error_setv(errp, src, line, func, err_class, fmt, ap);
     va_end(ap);
 }
 
-void error_setg(Error **errp, const char *fmt, ...)
+void error_setg_internal(Error **errp,
+                         const char *src, int line, const char *func,
+                         const char *fmt, ...)
 {
     va_list ap;
 
     va_start(ap, fmt);
-    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 }
 
-void error_setg_errno(Error **errp, int os_errno, const char *fmt, ...)
+void error_setg_errno_internal(Error **errp,
+                               const char *src, int line, const char *func,
+                               int os_errno, const char *fmt, ...)
 {
     va_list ap;
     char *msg;
@@ -76,7 +95,7 @@ void error_setg_errno(Error **errp, int os_errno, const char *fmt, ...)
     }
 
     va_start(ap, fmt);
-    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 
     if (os_errno != 0) {
@@ -88,14 +107,19 @@ void error_setg_errno(Error **errp, int os_errno, const char *fmt, ...)
     errno = saved_errno;
 }
 
-void error_setg_file_open(Error **errp, int os_errno, const char *filename)
+void error_setg_file_open_internal(Error **errp,
+                                   const char *src, int line, const char *func,
+                                   int os_errno, const char *filename)
 {
-    error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
+    error_setg_errno_internal(errp, src, line, func, os_errno,
+                              "Could not open '%s'", filename);
 }
 
 #ifdef _WIN32
 
-void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
+void error_setg_win32_internal(Error **errp,
+                               const char *src, int line, const char *func,
+                               int win32_err, const char *fmt, ...)
 {
     va_list ap;
     char *msg1, *msg2;
@@ -105,7 +129,7 @@ void error_setg_win32(Error **errp, int win32_err, const char *fmt, ...)
     }
 
     va_start(ap, fmt);
-    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 
     if (win32_err != 0) {
@@ -158,8 +182,7 @@ void error_free(Error *err)
 void error_propagate(Error **dst_errp, Error *local_err)
 {
     if (local_err && dst_errp == &error_abort) {
-        error_report_err(local_err);
-        abort();
+        error_do_abort(local_err);
     } else if (dst_errp && !*dst_errp) {
         *dst_errp = local_err;
     } else if (local_err) {
-- 
2.4.3

Re: [Qemu-devel] [PULL 0/7] error: On abort, report where the error was created

[Peter Maydell]

On 10 September 2015 at 14:32, Markus Armbruster <armbru@redhat.com> wrote:
> The following changes since commit fc04a730b7e60f4a62d6260d4eb9c537d1d3643f:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20150908' into staging (2015-09-08 18:02:36 +0100)
>
> are available in the git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-error-2015-09-10
>
> for you to fetch changes up to 1e9b65bb1bad51735cab6c861c29b592dccabf0e:
>
>   error: On abort, report where the error was created (2015-09-10 13:48:06 +0200)
>
> ----------------------------------------------------------------
> error: On abort, report where the error was created

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 7/7] error: On abort, report where the error was created

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On 09/10/2015 07:32 AM, Markus Armbruster wrote:
> This is particularly useful when we abort in error_propagate(),
> because there the stack backtrace doesn't lead to where the error was
> created.  Looks like this:
> 
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---

Already in mainline, but I'm wondering...


> +++ b/util/error.c
> @@ -18,12 +18,23 @@ struct Error
>  {
>      char *msg;
>      ErrorClass err_class;
> +    const char *src, *func;
> +    int line;
>  };
>  

Should we also be modifying error_copy() to propagate source information
into the copy?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PULL 7/7] error: On abort, report where the error was created

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On 09/10/2015 10:21 AM, Eric Blake wrote:

> Already in mainline, but I'm wondering...
> 
> 
>> +++ b/util/error.c
>> @@ -18,12 +18,23 @@ struct Error
>>  {
>>      char *msg;
>>      ErrorClass err_class;
>> +    const char *src, *func;
>> +    int line;
>>  };
>>  
> 
> Should we also be modifying error_copy() to propagate source information
> into the copy?

Rather than just wondering, I've proposed a patch:
http://thread.gmane.org/gmane.comp.emulators.qemu/361110/focus=361117

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]