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 <arm...@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