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

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

[Markus Armbruster]

Applies on top of "[PULL 00/24] Monitor patches".

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        | 226 ++++++++++++++++++++++++++++++--------------
 qga/vss-win32.c             |   6 +-
 qga/vss-win32/requester.cpp |   8 +-
 qga/vss-win32/requester.h   |  12 ++-
 util/error.c                | 111 +++++++++++-----------
 5 files changed, 228 insertions(+), 135 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 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>
---
 util/error.c | 69 ++++++++++++++++++++++--------------------------------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/util/error.c b/util/error.c
index 14f4351..19982b1 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,24 @@ 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", msg1, msg2);
         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 1/7] error: De-duplicate code creating Error objects

[Eric Blake]

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

On 06/22/2015 01:26 PM, 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>
> ---
>  util/error.c | 69 ++++++++++++++++++++++--------------------------------------
>  1 file changed, 25 insertions(+), 44 deletions(-)

> @@ -96,37 +90,24 @@ 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, ...)
>  {

>      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", msg1, msg2);

Loses " (error: %x)".  Do we care?  I don't, except maybe in the commit
message...

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/22/2015 01:26 PM, 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>
>> ---
>>  util/error.c | 69 ++++++++++++++++++++++--------------------------------------
>>  1 file changed, 25 insertions(+), 44 deletions(-)
>
>> @@ -96,37 +90,24 @@ 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, ...)
>>  {
>
>>      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", msg1, msg2);
>
> Loses " (error: %x)".  Do we care?  I don't, except maybe in the commit
> message...

Unintentional, I'll put it right back.

(I don't care either, but dropping it deserves its own commit)

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

Thanks!

[Qemu-devel] [PATCH 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>
---
 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 19982b1..7b687f6 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

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

[Eric Blake]

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

On 06/22/2015 01:26 PM, Markus Armbruster wrote:
> Saves a tiny amount of code at every call site.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/error.h | 4 ++--
>  util/error.c         | 9 +++++++++
>  2 files changed, 11 insertions(+), 2 deletions(-)

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

(Interesting that you turn it back into a macro later in the series...)

-- 
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 2/7] error: Make error_setg() a function

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/22/2015 01:26 PM, Markus Armbruster wrote:
>> Saves a tiny amount of code at every call site.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/error.h | 4 ++--
>>  util/error.c         | 9 +++++++++
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> (Interesting that you turn it back into a macro later in the series...)

Yes, in PATCH 7, but even then error_setg_internal() remains a function,
not a macro wrapper around error_set_internal(), thus we still save code
to pass a 0 argument at every call site.

Of course, PATCH 7 increases code size quite a bit more than this patch
decreases it: this patch deletes "pass 0 argument", PATCH 7 adds "pass
__FILE__ and __LINE__ arguments".  Plus the __FILE_ literals, but those
should be in cold memory.

If the increase bothers us, we could add suitable #ifdeffery to
optionally suppress it.  Seems not worth it to me.

[Qemu-devel] [PATCH 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>
---
 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 7b687f6..48eb53b 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

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

[Eric Blake]

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

On 06/22/2015 01:26 PM, Markus Armbruster wrote:
> 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.

Better than some other horrid casts we do, like monitor.c playing fast
and loose with 'Monitor *' vs. 'FILE *' in hmp_info_mtree() and friends.
 But that's a cleanup for another series.

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

-- 
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 3/7] qga: Clean up unnecessarily dirty casts

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/22/2015 01:26 PM, Markus Armbruster wrote:
>> 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.
>
> Better than some other horrid casts we do, like monitor.c playing fast
> and loose with 'Monitor *' vs. 'FILE *' in hmp_info_mtree() and friends.
>  But that's a cleanup for another series.

I wonder why we don't have a warning to flag casts of a function where
calling the result would be undefined behavior.

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

Thanks!

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

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

[Eric Blake]

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

On 06/22/2015 01:26 PM, Markus Armbruster wrote:
> 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>
> ---
>  qga/vss-win32.c             | 1 +
>  qga/vss-win32/requester.cpp | 3 ++-
>  qga/vss-win32/requester.h   | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)

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 --]

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@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 48eb53b..fb575ac 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

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

[Eric Blake]

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

On 06/22/2015 01:26 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/error.h | 7 ++-----
>  util/error.c         | 5 ++---
>  2 files changed, 4 insertions(+), 8 deletions(-)

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 --]

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 176 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 126 insertions(+), 50 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 8c3a7dd..9466b09 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 losely 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 passing it on, please use
+ *     foo(arg, errp);
+ * for readability.
+ */
+
 #ifndef ERROR_H
 #define ERROR_H
 
@@ -16,85 +78,99 @@
 #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.
  */
 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 6/7] error: Revamp interface documentation

[Eric Blake]

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

On 06/22/2015 01:26 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/error.h | 176 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 126 insertions(+), 50 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 8c3a7dd..9466b09 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>,

Trailing comma, but not intermediate, looks weird.

>   *
>   * 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 losely patterned after Glib's GError.

s/losely/loosely/


> + *
> + * Receive an error and pass it on to the caller

s/$/:/

> + *
> + * But when all you do with the error is passing it on, please use

s/passing/pass/


> +/*
> + * 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

s/dst_errp,/dst_errp/

> -/**
> - * Free an error object.
> +/*
> + * Free @err.
>   */
>  void error_free(Error *err);

Is error_free(NULL) safe? Worth documenting (because it affects
paradigms used in cleanup labels).

Overall, a definite improvement.

-- 
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 6/7] error: Revamp interface documentation

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/22/2015 01:26 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/error.h | 176 ++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 126 insertions(+), 50 deletions(-)
>> 
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 8c3a7dd..9466b09 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>,
>
> Trailing comma, but not intermediate, looks weird.

Fixing...

>>   *
>>   * 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 losely patterned after Glib's GError.
>
> s/losely/loosely/

Fixing...

>> + *
>> + * Receive an error and pass it on to the caller
>
> s/$/:/

Fixing...

>> + *
>> + * But when all you do with the error is passing it on, please use
>
> s/passing/pass/

Native speaker's advice accepted :)

>> +/*
>> + * 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
>
> s/dst_errp,/dst_errp/

Fixing...

>> -/**
>> - * Free an error object.
>> +/*
>> + * Free @err.
>>   */
>>  void error_free(Error *err);
>
> Is error_free(NULL) safe? Worth documenting (because it affects
> paradigms used in cleanup labels).

Yes, it's safe, and yes, it's worth documenting, so I'll do it in v2.

> Overall, a definite improvement.

Thanks!

[Qemu-devel] [PATCH 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 at /work/armbru/qemu/blockdev.c:322:
    qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid write error action
    Aborted (core dumped)
    [Exit 134 (SIGABRT)]

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__ 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 14KiB for me (0.3%).  Tolerable.  Could be
less if we passed relative rather than absolute source file names to
the compiler.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h        | 36 +++++++++++++++++++++++++---------
 qga/vss-win32.c             |  2 +-
 qga/vss-win32/requester.cpp |  5 +++--
 qga/vss-win32/requester.h   |  5 +++--
 util/error.c                | 47 ++++++++++++++++++++++++++++++---------------
 5 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 9466b09..501e110 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -104,16 +104,22 @@ 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__, (fmt), ## __VA_ARGS__)
+void error_setg_internal(Error **errp, const char *src, int line,
+                         const char *fmt, ...) GCC_FMT_ATTR(4, 5);
 
 /*
  * 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__, (os_error),   \
+                              (fmt), ## __VA_ARGS__)
+void error_setg_errno_internal(Error **errp, const char *fname, int line,
+                               int os_error, const char *fmt, ...)
+    GCC_FMT_ATTR(5, 6);
 
 #ifdef _WIN32
 /*
@@ -121,8 +127,12 @@ 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__, (win32_err),  \
+                              (fmt), ## __VA_ARGS__)
+void error_setg_win32_internal(Error **errp, const char *src, int line,
+                               int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(5, 6);
 #endif
 
 /*
@@ -143,7 +153,11 @@ 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__,   \
+                                  (os_errno), (filename))
+void error_setg_file_open_internal(Error **errp, const char *src, int line,
+                                   int os_errno, const char *filename);
 
 /*
  * Return an exact copy of @err.
@@ -165,8 +179,12 @@ 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__, (err_class), \
+                       (fmt), ## __VA_ARGS__)
+void error_set_internal(Error **errp, const char *src, int line,
+                        ErrorClass err_class, const char *fmt, ...)
+    GCC_FMT_ATTR(5, 6);
 
 /*
  * 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..0373491 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__,       \
+                                    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..7dd8102 100644
--- a/qga/vss-win32/requester.h
+++ b/qga/vss-win32/requester.h
@@ -23,8 +23,9 @@ 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,
+                             int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(5, 6);
 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 fb575ac..a655cfe 100644
--- a/util/error.c
+++ b/util/error.c
@@ -18,12 +18,21 @@ struct Error
 {
     char *msg;
     ErrorClass err_class;
+    const char *src;
+    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 at %s:%d:\n", err->src, err->line);
+    error_report_err(err);
+    abort();
+}
+
+static void error_setv(Error **errp, const char *src, int line,
+                       ErrorClass err_class, const char *fmt, va_list ap)
 {
     Error *err;
     int saved_errno = errno;
@@ -36,10 +45,11 @@ 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;
 
     if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
+        error_do_abort(err);
     }
 
     *errp = err;
@@ -47,25 +57,28 @@ 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,
+                        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, 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 *fmt, ...)
 {
     va_list ap;
 
     va_start(ap, fmt);
-    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, 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,
+                              int os_errno, const char *fmt, ...)
 {
     va_list ap;
     char *msg;
@@ -76,7 +89,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, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 
     if (os_errno != 0) {
@@ -88,14 +101,17 @@ 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,
+                                   int os_errno, const char *filename)
 {
-    error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
+    error_setg_errno_internal(errp, src, line, 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,
+                              int win32_err, const char *fmt, ...)
 {
     va_list ap;
     char *msg1, *msg2;
@@ -105,7 +121,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, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
 
     if (win32_err != 0) {
@@ -157,8 +173,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 7/7] error: On abort, report where the error was created

[Laszlo Ersek]

On 06/22/15 21:26, 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 at /work/armbru/qemu/blockdev.c:322:
>     qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid write error action
>     Aborted (core dumped)
>     [Exit 134 (SIGABRT)]
> 
> 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__ arguments.  Not exactly pretty, but
> it works.

Please consider squeezing in __func__ too. The information given by
__FILE__:__LINE__ goes stale quite a bit faster than when __func__ is
included (in a triplet then).

In a poorly written bug report (eg. no exact version / git commit
identified), the function name could be the most helpful bit.

Just an idea, of course. :)

Thanks
Laszlo

> 
> 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 14KiB for me (0.3%).  Tolerable.  Could be
> less if we passed relative rather than absolute source file names to
> the compiler.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/error.h        | 36 +++++++++++++++++++++++++---------
>  qga/vss-win32.c             |  2 +-
>  qga/vss-win32/requester.cpp |  5 +++--
>  qga/vss-win32/requester.h   |  5 +++--
>  util/error.c                | 47 ++++++++++++++++++++++++++++++---------------
>  5 files changed, 65 insertions(+), 30 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 9466b09..501e110 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -104,16 +104,22 @@ 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__, (fmt), ## __VA_ARGS__)
> +void error_setg_internal(Error **errp, const char *src, int line,
> +                         const char *fmt, ...) GCC_FMT_ATTR(4, 5);
>  
>  /*
>   * 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__, (os_error),   \
> +                              (fmt), ## __VA_ARGS__)
> +void error_setg_errno_internal(Error **errp, const char *fname, int line,
> +                               int os_error, const char *fmt, ...)
> +    GCC_FMT_ATTR(5, 6);
>  
>  #ifdef _WIN32
>  /*
> @@ -121,8 +127,12 @@ 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__, (win32_err),  \
> +                              (fmt), ## __VA_ARGS__)
> +void error_setg_win32_internal(Error **errp, const char *src, int line,
> +                               int win32_err, const char *fmt, ...)
> +    GCC_FMT_ATTR(5, 6);
>  #endif
>  
>  /*
> @@ -143,7 +153,11 @@ 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__,   \
> +                                  (os_errno), (filename))
> +void error_setg_file_open_internal(Error **errp, const char *src, int line,
> +                                   int os_errno, const char *filename);
>  
>  /*
>   * Return an exact copy of @err.
> @@ -165,8 +179,12 @@ 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__, (err_class), \
> +                       (fmt), ## __VA_ARGS__)
> +void error_set_internal(Error **errp, const char *src, int line,
> +                        ErrorClass err_class, const char *fmt, ...)
> +    GCC_FMT_ATTR(5, 6);
>  
>  /*
>   * 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..0373491 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__,       \
> +                                    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..7dd8102 100644
> --- a/qga/vss-win32/requester.h
> +++ b/qga/vss-win32/requester.h
> @@ -23,8 +23,9 @@ 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,
> +                             int win32_err, const char *fmt, ...)
> +    GCC_FMT_ATTR(5, 6);
>  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 fb575ac..a655cfe 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -18,12 +18,21 @@ struct Error
>  {
>      char *msg;
>      ErrorClass err_class;
> +    const char *src;
> +    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 at %s:%d:\n", err->src, err->line);
> +    error_report_err(err);
> +    abort();
> +}
> +
> +static void error_setv(Error **errp, const char *src, int line,
> +                       ErrorClass err_class, const char *fmt, va_list ap)
>  {
>      Error *err;
>      int saved_errno = errno;
> @@ -36,10 +45,11 @@ 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;
>  
>      if (errp == &error_abort) {
> -        error_report_err(err);
> -        abort();
> +        error_do_abort(err);
>      }
>  
>      *errp = err;
> @@ -47,25 +57,28 @@ 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,
> +                        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, 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 *fmt, ...)
>  {
>      va_list ap;
>  
>      va_start(ap, fmt);
> -    error_setv(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
> +    error_setv(errp, src, line, 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,
> +                              int os_errno, const char *fmt, ...)
>  {
>      va_list ap;
>      char *msg;
> @@ -76,7 +89,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, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
>      va_end(ap);
>  
>      if (os_errno != 0) {
> @@ -88,14 +101,17 @@ 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,
> +                                   int os_errno, const char *filename)
>  {
> -    error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
> +    error_setg_errno_internal(errp, src, line, 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,
> +                              int win32_err, const char *fmt, ...)
>  {
>      va_list ap;
>      char *msg1, *msg2;
> @@ -105,7 +121,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, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
>      va_end(ap);
>  
>      if (win32_err != 0) {
> @@ -157,8 +173,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) {
> 

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

[Eric Blake]

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

On 06/22/2015 01:26 PM, 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 at /work/armbru/qemu/blockdev.c:322:
>     qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid write error action
>     Aborted (core dumped)
>     [Exit 134 (SIGABRT)]
> 
> 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__ arguments.  Not exactly pretty, but
> it works.

I agree with Laszlo that adding __func__ to the mix also helps.

> 
> 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 14KiB for me (0.3%).  Tolerable.  Could be
> less if we passed relative rather than absolute source file names to
> the compiler.

I also like it.

> +#define error_setg(errp, fmt, ...) \
> +    error_setg_internal((errp), __FILE__, __LINE__, (fmt), ## __VA_ARGS__)
> +void error_setg_internal(Error **errp, const char *src, int line,
> +                         const char *fmt, ...) GCC_FMT_ATTR(4, 5);
>  

> +#define error_setg_errno(errp, os_error, fmt, ...)                      \
> +    error_setg_errno_internal((errp), __FILE__, __LINE__, (os_error),   \
> +                              (fmt), ## __VA_ARGS__)

Nit - why the difference in \ alignment?

Nit - as used here, 'errp', 'fmt', and 'os_error' can be used
unambiguously; you don't need '(errp)' given the context of a
parenthesized comma-separated list (even if someone DID want to unusual
by passing in '(a,b)' with a comma operator for their 'errp' argument,
they'd have to supply the () because of the semantics of making the
macro call).

Nit - '## __VA_ARGS__' is a gcc-ism and not portable C99; but I think
clang supports it, and we don't really care about other compilers at the
moment. At any rate, we already use it elsewhere in qemu.git.

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/22/2015 01:26 PM, 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 at /work/armbru/qemu/blockdev.c:322:
>>     qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid
>> write error action
>>     Aborted (core dumped)
>>     [Exit 134 (SIGABRT)]
>> 
>> 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__ arguments.  Not exactly pretty, but
>> it works.
>
> I agree with Laszlo that adding __func__ to the mix also helps.

Okay, I'll give it a try.

>> 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 14KiB for me (0.3%).  Tolerable.  Could be
>> less if we passed relative rather than absolute source file names to
>> the compiler.
>
> I also like it.
>
>> +#define error_setg(errp, fmt, ...) \
>> +    error_setg_internal((errp), __FILE__, __LINE__, (fmt), ## __VA_ARGS__)
>> +void error_setg_internal(Error **errp, const char *src, int line,
>> +                         const char *fmt, ...) GCC_FMT_ATTR(4, 5);
>>  
>
>> +#define error_setg_errno(errp, os_error, fmt, ...)                      \
>> +    error_setg_errno_internal((errp), __FILE__, __LINE__, (os_error),   \
>> +                              (fmt), ## __VA_ARGS__)
>
> Nit - why the difference in \ alignment?

I'm dense today... difference between where and where?

> Nit - as used here, 'errp', 'fmt', and 'os_error' can be used
> unambiguously; you don't need '(errp)' given the context of a
> parenthesized comma-separated list (even if someone DID want to unusual
> by passing in '(a,b)' with a comma operator for their 'errp' argument,
> they'd have to supply the () because of the semantics of making the
> macro call).

I put parenthesis around macro parameters in the expansion pretty much
unthinkingly, because thought is expensive :)

> Nit - '## __VA_ARGS__' is a gcc-ism and not portable C99; but I think
> clang supports it, and we don't really care about other compilers at the
> moment. At any rate, we already use it elsewhere in qemu.git.

Correct.  For what it's worth, ./HACKING recommends it.

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

[Eric Blake]

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

On 07/22/2015 07:54 AM, Markus Armbruster wrote:

>>
>>> +#define error_setg(errp, fmt, ...) \
>>> +    error_setg_internal((errp), __FILE__, __LINE__, (fmt), ## __VA_ARGS__)
>>> +void error_setg_internal(Error **errp, const char *src, int line,
>>> +                         const char *fmt, ...) GCC_FMT_ATTR(4, 5);
>>>  
>>
>>> +#define error_setg_errno(errp, os_error, fmt, ...)                      \
>>> +    error_setg_errno_internal((errp), __FILE__, __LINE__, (os_error),   \
>>> +                              (fmt), ## __VA_ARGS__)
>>
>> Nit - why the difference in \ alignment?
> 
> I'm dense today... difference between where and where?

one space after error_setg(), aligned to far right after error_setg_errno().

> 
>> Nit - as used here, 'errp', 'fmt', and 'os_error' can be used
>> unambiguously; you don't need '(errp)' given the context of a
>> parenthesized comma-separated list (even if someone DID want to unusual
>> by passing in '(a,b)' with a comma operator for their 'errp' argument,
>> they'd have to supply the () because of the semantics of making the
>> macro call).
> 
> I put parenthesis around macro parameters in the expansion pretty much
> unthinkingly, because thought is expensive :)

Of course, it doesn't hurt semantically to leave them in, and if you
value the lower maintenance burden of less thinking, then the extra
typing is not something I will reject :)

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/22/2015 07:54 AM, Markus Armbruster wrote:
>
>>>
>>>> +#define error_setg(errp, fmt, ...) \
>>>> +    error_setg_internal((errp), __FILE__, __LINE__, (fmt), ## __VA_ARGS__)
>>>> +void error_setg_internal(Error **errp, const char *src, int line,
>>>> +                         const char *fmt, ...) GCC_FMT_ATTR(4, 5);
>>>>  
>>>
>>>> +#define error_setg_errno(errp, os_error, fmt, ...)                      \
>>>> +    error_setg_errno_internal((errp), __FILE__, __LINE__, (os_error),   \
>>>> +                              (fmt), ## __VA_ARGS__)
>>>
>>> Nit - why the difference in \ alignment?
>> 
>> I'm dense today... difference between where and where?
>
> one space after error_setg(), aligned to far right after error_setg_errno().

Normalized.

>>> Nit - as used here, 'errp', 'fmt', and 'os_error' can be used
>>> unambiguously; you don't need '(errp)' given the context of a
>>> parenthesized comma-separated list (even if someone DID want to unusual
>>> by passing in '(a,b)' with a comma operator for their 'errp' argument,
>>> they'd have to supply the () because of the semantics of making the
>>> macro call).
>> 
>> I put parenthesis around macro parameters in the expansion pretty much
>> unthinkingly, because thought is expensive :)
>
> Of course, it doesn't hurt semantically to leave them in, and if you
> value the lower maintenance burden of less thinking, then the extra
> typing is not something I will reject :)

Thanks!

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

[Michael S. Tsirkin]

On Mon, Jun 22, 2015 at 09:26:33PM +0200, Markus Armbruster wrote:
> Applies on top of "[PULL 00/24] Monitor patches".

I've no objection to this, though I'd still like
to make it abort at the actuall error site too.
That one will need a bit more work though.

> 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        | 226 ++++++++++++++++++++++++++++++--------------
>  qga/vss-win32.c             |   6 +-
>  qga/vss-win32/requester.cpp |   8 +-
>  qga/vss-win32/requester.h   |  12 ++-
>  util/error.c                | 111 +++++++++++-----------
>  5 files changed, 228 insertions(+), 135 deletions(-)
> 
> -- 
> 1.9.3