| Message ID | 1435001200-20610-8-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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) { >
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> 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.
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> 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!
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) {
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(-)