From 552375088a832fd5945ede92d01f98977b4eca13 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 19 Jun 2015 13:59:47 +0200 Subject: [PATCH 1/7] error: De-duplicate code creating Error objects 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 Reviewed-by: Eric Blake --- util/error.c | 68 +++++++++++++++++++--------------------------------- 1 file changed, 25 insertions(+), 43 deletions(-) diff --git a/util/error.c b/util/error.c index 14f4351879..4518642881 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,39 +47,36 @@ 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); - if (os_errno != 0) { - err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno)); - g_free(msg1); - } else { - err->msg = msg1; - } + error_setv(errp, err_class, fmt, ap); va_end(ap); - err->err_class = err_class; - if (errp == &error_abort) { - error_report_err(err); - abort(); + if (os_errno != 0) { + msg = (*errp)->msg; + (*errp)->msg = g_strdup_printf("%s: %s", msg, strerror(os_errno)); + g_free(msg); } - *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 From a9499ddd82a99c66cc72a08e72427c423acfea1c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 19 Jun 2015 15:36:16 +0200 Subject: [PATCH 2/7] error: Make error_setg() a function Saves a tiny amount of code at every call site. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- 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 f44c451830..34af4e103f 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 4518642881..8f12f67012 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, ...) { From e7cf59e84767e30b507b6bd7c1347072ec12b636 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 19 Jun 2015 20:44:54 +0200 Subject: [PATCH 3/7] qga: Clean up unnecessarily dirty casts 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 Reviewed-by: Eric Blake --- 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 34af4e103f..692e01346e 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 0e4095736e..e1f539835c 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 922e74ddfc..b130fee934 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 374f9b8d16..0a8d048874 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 8f12f67012..9620f2a1f6 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) { From 08e64640357cd9517aa30fd49840f05f0f2ee3a4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Sat, 20 Jun 2015 09:33:56 +0200 Subject: [PATCH 4/7] qga/vss-win32: Document the DLL requires non-null errp 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 Reviewed-by: Eric Blake --- 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 e1f539835c..d75d7bbf09 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 b130fee934..aae0d5f2bf 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 0a8d048874..34be5c1d11 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); From 4463dcb85c9f992f0c4d93f2142c8d64dcc85c5c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 19 Jun 2015 22:16:14 +0200 Subject: [PATCH 5/7] error: error_set_errno() is unused, drop Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- 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 692e01346e..8c3a7ddf09 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 9620f2a1f6..b842f2040c 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) { From edf6f3b3358597d37da0cf636ce3ed8a546d0f26 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 19 Jun 2015 18:29:24 +0200 Subject: [PATCH 6/7] error: Revamp interface documentation Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- 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 8c3a7ddf09..358a9d39a4 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 + * Markus Armbruster * * 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 -/** - * 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 From 1e9b65bb1bad51735cab6c861c29b592dccabf0e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 19 Jun 2015 19:21:59 +0200 Subject: [PATCH 7/7] error: On abort, report where the error was created This is particularly useful when we abort in error_propagate(), because there the stack backtrace doesn't lead to where the error was created. Looks like this: Unexpected error in parse_block_error_action() at .../qemu/blockdev.c:322: qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid write error action Aborted (core dumped) Note: to get this example output, I monkey-patched drive_new() to pass &error_abort to blockdev_init(). To keep the error handling boiler plate from growing even more, all error_setFOO() become macros expanding into error_setFOO_internal() with additional __FILE__, __LINE__, __func__ arguments. Not exactly pretty, but it works. The macro trickery breaks down when you take the address of an error_setFOO(). Fortunately, we do that in just one place: qemu-ga's Windows VSS provider and requester DLL wants to call error_setg_win32() through a function pointer "to avoid linking glib to the DLL". Use error_setg_win32_internal() there. The use of the function pointer is already wrapped in a macro, so the churn isn't bad. Code size increases by some 35KiB for me (0.7%). Tolerable. Could be less if we passed relative rather than absolute source file names to the compiler, or forwent reporting __func__. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Acked-by: Laszlo Ersek --- 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 358a9d39a4..426d5eaceb 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 d75d7bbf09..2142b4964d 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 aae0d5f2bf..9b3e310971 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 34be5c1d11..c3093cf4b6 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 b842f2040c..cdb726ce81 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) {