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

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

[Markus Armbruster]

v2:
* Trivially rebased.
* [PATCH 1] Fix accidental change of error_set_win32() message [Eric]
* [PATCH 6] Correct typoes, document error_free(NULL) is safe [Eric]
* [PATCH 7] Report __func__, too [Eric, László]
  This more than doubles the text size increase: 35KiB (0.7%) instead
  of 14KiB (0.3%), but much of it should be cache-cold memory.

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(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 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
-- 
1.9.3

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

[Eric Blake]

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

On 07/23/2015 08:01 AM, Markus Armbruster wrote:
> 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(-)
> 

>      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));

Unrelated comment, so doesn't change R-b:

strerror() is not required to be thread-safe, and can return NULL on
error.  Do we care?

-- 
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] [PATCH v2 1/7] error: De-duplicate code creating Error objects

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/23/2015 08:01 AM, Markus Armbruster wrote:
>> 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(-)
>> 
>
>>      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));
>
> Unrelated comment, so doesn't change R-b:
>
> strerror() is not required to be thread-safe, and can return NULL on
> error.  Do we care?

Quoting strerror(3):

       POSIX.1-2001 permits strerror() to set errno if the call
       encounters an error, but does not specify what value should be
       returned as the function result in the event of an error.  On
       some systems, strerror() returns NULL if the error number is
       unknown.  On other systems, strerror() returns a string something
       like "Error nnn occurred" and sets errno to EINVAL if the error
       number is unknown.  C99 and POSIX.1-2008 require the return value
       to be non-NULL.

We already rely on strerror() behaving nicely in many, many places.

Let's not start worrying about rotten, NULL-returning implementations
until we run into one.

I don't want to hand-wave thread safety worries away similarly.
However, this patch doesn't add strerror() uses.  If we want to discuss
the existing potential problems, I feel we should start a new thread.

[Qemu-devel] [PATCH v2 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, ...)
 {
-- 
1.9.3

[Qemu-devel] [PATCH v2 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) {
-- 
1.9.3

[Qemu-devel] [PATCH v2 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);
-- 
1.9.3

[Qemu-devel] [PATCH v2 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) {
-- 
1.9.3

[Qemu-devel] [PATCH v2 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..7d808e8 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
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 6/7] error: Revamp interface documentation

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> 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..7d808e8 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.

Excellent; it's great to have this documented.
Do we have a note anywhere that says why we don't just use GError?

Dave

> + *
> + * 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
> -- 
> 1.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 6/7] error: Revamp interface documentation

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> 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..7d808e8 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.
>
> Excellent; it's great to have this documented.

Thanks!

> Do we have a note anywhere that says why we don't just use GError?

I'm not aware of such a note, and I don't know / remember why we didn't
use GError.  Perhaps one of the guys signing off the initial commit
does (cc'ing them):

    commit d5ec4f27c387c3b3200abb8656343b2519ea3047
    Author: Luiz Capitulino <lcapitulino@redhat.com>
    Date:   Wed Jun 1 12:14:49 2011 -0500

        Introduce the new error framework

        New error-handling framework that allows for exception-like error
        propagation.

        Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
        Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
        Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Here's a possible clue:

        GLib is an extremely common library that has a portable thread implementatiocommit e18df14185e817ba735bce57ecdef9a55fb3d093
    Author: Anthony Liguori <aliguori@us.ibm.com>
    Date:   Tue Jul 19 14:50:29 2011 -0500

        Add hard build dependency on glib
[...]
        Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
        Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
        Signed-off-by: Luiz Capitulino <lcapitulino@gmail.com>

Error predates our use of GLib.

Re: [Qemu-devel] [PATCH v2 6/7] error: Revamp interface documentation

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> 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..7d808e8 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.
> >
> > Excellent; it's great to have this documented.
> 
> Thanks!
> 
> > Do we have a note anywhere that says why we don't just use GError?
> 
> I'm not aware of such a note, and I don't know / remember why we didn't
> use GError.  Perhaps one of the guys signing off the initial commit
> does (cc'ing them):
> 
>     commit d5ec4f27c387c3b3200abb8656343b2519ea3047
>     Author: Luiz Capitulino <lcapitulino@redhat.com>
>     Date:   Wed Jun 1 12:14:49 2011 -0500
> 
>         Introduce the new error framework
> 
>         New error-handling framework that allows for exception-like error
>         propagation.
> 
>         Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>         Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>         Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Here's a possible clue:
> 
>         GLib is an extremely common library that has a portable thread implementatiocommit e18df14185e817ba735bce57ecdef9a55fb3d093
>     Author: Anthony Liguori <aliguori@us.ibm.com>
>     Date:   Tue Jul 19 14:50:29 2011 -0500
> 
>         Add hard build dependency on glib
> [...]
>         Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>         Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>         Signed-off-by: Luiz Capitulino <lcapitulino@gmail.com>
> 
> Error predates our use of GLib.

By a month!
Are there semantics close enough that we could typedef Error to GError and then
slowly replace the use of our functions with standard glib calls?

It would just end up as one chunk less of code to maintain, and
something that new people wouldn't have to learn if they already new glib.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 6/7] error: Revamp interface documentation

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> 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..7d808e8 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.
>> >
>> > Excellent; it's great to have this documented.
>> 
>> Thanks!
>> 
>> > Do we have a note anywhere that says why we don't just use GError?
>> 
>> I'm not aware of such a note, and I don't know / remember why we didn't
>> use GError.  Perhaps one of the guys signing off the initial commit
>> does (cc'ing them):
>> 
>>     commit d5ec4f27c387c3b3200abb8656343b2519ea3047
>>     Author: Luiz Capitulino <lcapitulino@redhat.com>
>>     Date:   Wed Jun 1 12:14:49 2011 -0500
>> 
>>         Introduce the new error framework
>> 
>>         New error-handling framework that allows for exception-like error
>>         propagation.
>> 
>>         Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>         Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>         Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> 
>> Here's a possible clue:
>> 
>>         GLib is an extremely common library that has a portable thread implementatiocommit e18df14185e817ba735bce57ecdef9a55fb3d093
>>     Author: Anthony Liguori <aliguori@us.ibm.com>
>>     Date:   Tue Jul 19 14:50:29 2011 -0500
>> 
>>         Add hard build dependency on glib
>> [...]
>>         Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>         Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>         Signed-off-by: Luiz Capitulino <lcapitulino@gmail.com>
>> 
>> Error predates our use of GLib.
>
> By a month!

"Knapp vorbei ist auch daneben."

> Are there semantics close enough that we could typedef Error to GError and then
> slowly replace the use of our functions with standard glib calls?
>
> It would just end up as one chunk less of code to maintain, and
> something that new people wouldn't have to learn if they already new glib.

There are obvious differences:

* GError uses (GQuark domain, gint code), we use ErrorClass (and regret
  it).

* GError lacks &error_abort.

* In g_propagate_error(&dest, src), dest must be null.
  error_propagate() throws away src then.

* More subtle differences may well exist.

Can't say offhand whether the obvious differences plus the ones I might
be missing make replacement impractical.

Can say that my appetite for tree-wide cleanups has limits %-}

error.[ch] weigh in at 167 SLOC.  Includes convenience function we'd
have to carry forward to GError.

[Qemu-devel] [PATCH v2 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 /work/armbru/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>
---
 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 7d808e8..6285ade 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..bb8cc2c 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..1dcea2b 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) {
-- 
1.9.3

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

[Eric Blake]

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

On 07/23/2015 08:01 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:
> 
>     Unexpected error in parse_block_error_action() at /work/armbru/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>
> ---
>  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(-)
> 

> +++ 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__))

Indentation looks odd here, but not fatal.

> -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, ...)

Indentation off again.

Those are minor, and could be fixed by maintainer.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] [PATCH v2 7/7] error: On abort, report where the error was created

[Laszlo Ersek]

On 07/23/15 16:47, Eric Blake wrote:
> On 07/23/2015 08:01 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:
>>
>>     Unexpected error in parse_block_error_action() at /work/armbru/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>
>> ---
>>  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(-)
>>
> 
>> +++ 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__))
> 
> Indentation looks odd here, but not fatal.
> 
>> -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, ...)
> 
> Indentation off again.
> 
> Those are minor, and could be fixed by maintainer.
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

I'm going to freeload in the wake of Eric's review, and just say "thank
you" for including __func__.

Acked-by: Laszlo Ersek <lersek@redhat.com>

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/23/2015 08:01 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:
>> 
>>     Unexpected error in parse_block_error_action() at /work/armbru/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>
>> ---
>>  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(-)
>> 
>
>> +++ 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__))
>
> Indentation looks odd here, but not fatal.
>
>> -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, ...)
>
> Indentation off again.
>
> Those are minor, and could be fixed by maintainer.

That would be me.  I can fix them up for my pull request.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!