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