| Message ID | 145442963860.1539.7135815311391731257.stgit@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I'm struggling with my review queue, and have had to resort to subsystem batching to increase throughput. Because of that, v3-v5 have flown by without a peep from me. My sincere apologies. Lluís Vilanova <vilanova@ac.upc.edu> writes: > Provide two lean functions to report error messages that fatal/abort > QEMU. > > Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> > --- > include/qemu/error-report.h | 19 +++++++++++++++++++ > util/qemu-error.c | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > index 7ab2355..6c2f142 100644 > --- a/include/qemu/error-report.h > +++ b/include/qemu/error-report.h > @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > const char *error_get_progname(void); > extern bool enable_timestamp_msg; > > +/* Report message and exit with error */ > +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); This lets people write things like error_report_fatal("The sky is falling"); instead of error_report("The sky is falling"); exit(1); or fprintf(stderr, "The sky is falling\n"); exit(1); I don't think that's an improvement in clarity. PATCH 3 actually does this for a couple of cases. > +/* Report message with caller location and abort */ > +#define error_vreport_abort(fmt, ap) \ > + do { \ > + error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \ > + error_vreport_abort_internal(fmt, ap); \ > + } while (0) > +#define error_report_abort(fmt, ...) \ > + do { \ > + error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \ > + error_report_abort_internal(fmt, ##__VA_ARGS__); \ > + } while (0) > + > +void error_report_abort_caller_internal(const char *file, int line, const char *func); > +void QEMU_NORETURN error_vreport_abort_internal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > +void QEMU_NORETURN error_report_abort_internal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > + > #endif Feature not mentioned in the commit message: your new form prints an additional line with source location information, like &error_abort does. See error_report_abort_internal() below. PATCH 4 uses error_report_abort() to change a dozen abort() to error_report_abort(... some message ...); or even error_report_abort(" "); Two aspects: source code and behavior. Source code: I don't think it makes it more readable. Behavior: prints a message before it crashes. The messages look like they make sense only to developers, not to users. This isn't surprising; these are internal errors, and you generally can't explain internal errors without referring to internal concepts the user doesn't know about. Should they happen, you need to debug anyway. As to the " " messages: surely you're joking, Mr. Feynman :) I feel the message adds very little information to the core dump for developers, and none for users. If the error message is genuinely useful to users, chances are we should exit(1) rather than abort(). If the QEMU developer community should decide I'm wrong and we really want to decorate abort()s with messages, we'd have to decorate the majority of the 600+ we have so that people can see the pattern. Without that, new undecorated ones will pop up faster than we can decorate them. In other words, the dozen you converted to demonstrate the idea for your RFC are a placeholder for one of these tree-wide transitions that are easier to start than to finish. PATCH 2 uses error_report_abort() to clean up error_setg(&error_abort, ... some message ...); to error_report_abort(... some message ...); I agree these uses of &error_abort should be cleaned up. However, I'd clean them up to abort() or assert(), for the reasons I just explained, and because that's what we do elsewhere. > diff --git a/util/qemu-error.c b/util/qemu-error.c > index ecf5708..3de002b 100644 > --- a/util/qemu-error.c > +++ b/util/qemu-error.c > @@ -237,3 +237,36 @@ void error_report(const char *fmt, ...) > error_vreport(fmt, ap); > va_end(ap); > } > + > +void error_vreport_fatal(const char *fmt, va_list ap) > +{ > + error_vreport(fmt, ap); > + exit(1); > +} > + > +void error_report_fatal(const char *fmt, ...) > +{ > + va_list ap; > + va_start(ap, fmt); > + error_vreport_fatal(fmt, ap); > + va_end(ap); > +} > + > +void error_report_abort_caller_internal(const char *file, int line, const char *func) > +{ > + error_report("Unexpected error in %s() at %s:%d:", func, file, line); Should use error_printf(), so we get Unexpected error in frobnicate() at frob.c:666: qemu-system-x86_64: --frobnicate: Out of frobs instead of qemu-system-x86_64: --frobnicate: Unexpected error in frobnicate() at frob.c:666: qemu-system-x86_64: --frobnicate: Out of frobs > +} > + > +void error_vreport_abort_internal(const char *fmt, va_list ap) > +{ > + error_vreport(fmt, ap); > + abort(); > +} > + > +void error_report_abort_internal(const char *fmt, ...) > +{ > + va_list ap; > + va_start(ap, fmt); > + error_vreport_abort_internal(fmt, ap); > + va_end(ap); > +}
On 02.02.2016 19:53, Markus Armbruster wrote: > Lluís Vilanova <vilanova@ac.upc.edu> writes: ... >> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h >> index 7ab2355..6c2f142 100644 >> --- a/include/qemu/error-report.h >> +++ b/include/qemu/error-report.h >> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> const char *error_get_progname(void); >> extern bool enable_timestamp_msg; >> >> +/* Report message and exit with error */ >> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); >> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > This lets people write things like > > error_report_fatal("The sky is falling"); > > instead of > > error_report("The sky is falling"); > exit(1); > > or > > fprintf(stderr, "The sky is falling\n"); > exit(1); > > I don't think that's an improvement in clarity. The problem is not the existing code, but that in a couple of new patches, I've now already seen that people are trying to use error_setg(&error_fatal, ... ); to do error reporting - which is IMHO the most ugliest way to do this, because I'd really not expect error_setg() to (always) exit QEMU when I quickly read through the sources. We fortunately do not have that in the sources yet (only some few spots with error_abort), but to prevent that people start using that, it would be good to have a error_report_fatal() function instead, I think. Alternatively, if you really want to see the exit(1) in the sources, I think this should be mentioned in the coding guidelines ... or are you fine with error_setg(&error_fatal, ...), Markus? Thomas
On Tue, Feb 02, 2016 at 10:47:35PM +0100, Thomas Huth wrote: > On 02.02.2016 19:53, Markus Armbruster wrote: > > Lluís Vilanova <vilanova@ac.upc.edu> writes: > ... > > >> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > >> index 7ab2355..6c2f142 100644 > >> --- a/include/qemu/error-report.h > >> +++ b/include/qemu/error-report.h > >> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > >> const char *error_get_progname(void); > >> extern bool enable_timestamp_msg; > >> > >> +/* Report message and exit with error */ > >> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > >> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > > > This lets people write things like > > > > error_report_fatal("The sky is falling"); > > > > instead of > > > > error_report("The sky is falling"); > > exit(1); > > > > or > > > > fprintf(stderr, "The sky is falling\n"); > > exit(1); > > > > I don't think that's an improvement in clarity. > > The problem is not the existing code, but that in a couple of new > patches, I've now already seen that people are trying to use > > error_setg(&error_fatal, ... ); So, I don't actually see any real advantage to error_report_fatal(...) over error_setg(&error_fatal, ...). > to do error reporting - which is IMHO the most ugliest way to do this, > because I'd really not expect error_setg() to (always) exit QEMU when I > quickly read through the sources. > We fortunately do not have that in the sources yet (only some few spots > with error_abort), but to prevent that people start using that, it would > be good to have a error_report_fatal() function instead, I think. > > Alternatively, if you really want to see the exit(1) in the sources, I > think this should be mentioned in the coding guidelines ... or are you > fine with error_setg(&error_fatal, ...), Markus? > > Thomas >
Thomas Huth <thuth@redhat.com> writes: > On 02.02.2016 19:53, Markus Armbruster wrote: >> Lluís Vilanova <vilanova@ac.upc.edu> writes: > ... > >>> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h >>> index 7ab2355..6c2f142 100644 >>> --- a/include/qemu/error-report.h >>> +++ b/include/qemu/error-report.h >>> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >>> const char *error_get_progname(void); >>> extern bool enable_timestamp_msg; >>> >>> +/* Report message and exit with error */ >>> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); >>> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> >> This lets people write things like >> >> error_report_fatal("The sky is falling"); >> >> instead of >> >> error_report("The sky is falling"); >> exit(1); >> >> or >> >> fprintf(stderr, "The sky is falling\n"); >> exit(1); >> >> I don't think that's an improvement in clarity. > > The problem is not the existing code, but that in a couple of new > patches, I've now already seen that people are trying to use > > error_setg(&error_fatal, ... ); > > to do error reporting - which is IMHO the most ugliest way to do this, > because I'd really not expect error_setg() to (always) exit QEMU when I > quickly read through the sources. I agree that this pattern is not wanted. > We fortunately do not have that in the sources yet (only some few spots > with error_abort), but to prevent that people start using that, it would > be good to have a error_report_fatal() function instead, I think. I doubt an error_report_fatal() function would be much better at stopping this pattern than the existing error_report(); exit(1) is. > Alternatively, if you really want to see the exit(1) in the sources, I > think this should be mentioned in the coding guidelines ... or are you > fine with error_setg(&error_fatal, ...), Markus? No, I'm not. I think this is a checkpatch job, supported by a suitable update to the docs in error.h.
David Gibson <david@gibson.dropbear.id.au> writes: > On Tue, Feb 02, 2016 at 10:47:35PM +0100, Thomas Huth wrote: >> On 02.02.2016 19:53, Markus Armbruster wrote: >> > Lluís Vilanova <vilanova@ac.upc.edu> writes: >> ... >> >> >> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h >> >> index 7ab2355..6c2f142 100644 >> >> --- a/include/qemu/error-report.h >> >> +++ b/include/qemu/error-report.h >> >> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> >> const char *error_get_progname(void); >> >> extern bool enable_timestamp_msg; >> >> >> >> +/* Report message and exit with error */ >> >> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); >> >> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> > >> > This lets people write things like >> > >> > error_report_fatal("The sky is falling"); >> > >> > instead of >> > >> > error_report("The sky is falling"); >> > exit(1); >> > >> > or >> > >> > fprintf(stderr, "The sky is falling\n"); >> > exit(1); >> > >> > I don't think that's an improvement in clarity. >> >> The problem is not the existing code, but that in a couple of new >> patches, I've now already seen that people are trying to use >> >> error_setg(&error_fatal, ... ); > > So, I don't actually see any real advantage to error_report_fatal(...) > over error_setg(&error_fatal, ...). I do. Compare: (a) error_report(...); exit(1); (b) error_report_fatal(...); (c) error_setg(&error_fatal, ...); In my opinion, (a) is clearest: even a relatively clueless reader will know what exit(1) does, can guess what error_report() approximately does, and doesn't need to know what it does exactly. (b) is slightly less obvious, and (c) is positively opaque. Let's stick to the obvious (a) and be done with it. [...]
On 03.02.2016 10:48, Markus Armbruster wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > >> On Tue, Feb 02, 2016 at 10:47:35PM +0100, Thomas Huth wrote: >>> On 02.02.2016 19:53, Markus Armbruster wrote: >>>> Lluís Vilanova <vilanova@ac.upc.edu> writes: >>> ... >>> >>>>> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h >>>>> index 7ab2355..6c2f142 100644 >>>>> --- a/include/qemu/error-report.h >>>>> +++ b/include/qemu/error-report.h >>>>> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >>>>> const char *error_get_progname(void); >>>>> extern bool enable_timestamp_msg; >>>>> >>>>> +/* Report message and exit with error */ >>>>> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); >>>>> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >>>> >>>> This lets people write things like >>>> >>>> error_report_fatal("The sky is falling"); >>>> >>>> instead of >>>> >>>> error_report("The sky is falling"); >>>> exit(1); >>>> >>>> or >>>> >>>> fprintf(stderr, "The sky is falling\n"); >>>> exit(1); >>>> >>>> I don't think that's an improvement in clarity. >>> >>> The problem is not the existing code, but that in a couple of new >>> patches, I've now already seen that people are trying to use >>> >>> error_setg(&error_fatal, ... ); >> >> So, I don't actually see any real advantage to error_report_fatal(...) >> over error_setg(&error_fatal, ...). > > I do. Compare: > > (a) error_report(...); > exit(1); > > (b) error_report_fatal(...); > > (c) error_setg(&error_fatal, ...); > > In my opinion, (a) is clearest: even a relatively clueless reader will > know what exit(1) does, can guess what error_report() approximately > does, and doesn't need to know what it does exactly. (b) is slightly > less obvious, and (c) is positively opaque. > > Let's stick to the obvious (a) and be done with it. Ok, (a) is fine for me too, as long as we avoid (c). Lluís, could you maybe add that information to your patch that updates the HACKING text? (and sorry for the fuzz with error_report_fatal() ... I thought it would be a good solution to avoid (c), but if (a) is preferred instead, then we should go with that solution instead). And, by the way, what about the spots that currently already use error_setg(&error_abort, ....) ? Should they be turned into error_report() + abort() instead? Or only abort(), without error message, since abort() is only about programming errors? Thomas
Markus Armbruster writes:
[...]
> As to the " " messages: surely you're joking, Mr. Feynman :)
BTW, completely off-topic, but I just wanted to say that's a great book :)
Cheers,
Lluis
On Wed, Feb 03, 2016 at 10:48:53AM +0100, Markus Armbruster wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > On Tue, Feb 02, 2016 at 10:47:35PM +0100, Thomas Huth wrote: > >> On 02.02.2016 19:53, Markus Armbruster wrote: > >> > Lluís Vilanova <vilanova@ac.upc.edu> writes: > >> ... > >> > >> >> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > >> >> index 7ab2355..6c2f142 100644 > >> >> --- a/include/qemu/error-report.h > >> >> +++ b/include/qemu/error-report.h > >> >> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > >> >> const char *error_get_progname(void); > >> >> extern bool enable_timestamp_msg; > >> >> > >> >> +/* Report message and exit with error */ > >> >> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > >> >> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > >> > > >> > This lets people write things like > >> > > >> > error_report_fatal("The sky is falling"); > >> > > >> > instead of > >> > > >> > error_report("The sky is falling"); > >> > exit(1); > >> > > >> > or > >> > > >> > fprintf(stderr, "The sky is falling\n"); > >> > exit(1); > >> > > >> > I don't think that's an improvement in clarity. > >> > >> The problem is not the existing code, but that in a couple of new > >> patches, I've now already seen that people are trying to use > >> > >> error_setg(&error_fatal, ... ); > > > > So, I don't actually see any real advantage to error_report_fatal(...) > > over error_setg(&error_fatal, ...). > > I do. Compare: > > (a) error_report(...); > exit(1); > > (b) error_report_fatal(...); > > (c) error_setg(&error_fatal, ...); > > In my opinion, (a) is clearest: even a relatively clueless reader will > know what exit(1) does, can guess what error_report() approximately > does, and doesn't need to know what it does exactly. (b) is slightly > less obvious, and (c) is positively opaque. > > Let's stick to the obvious (a) and be done with it. I'm fine with that, but I still see no real difference between (b) and (c).
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index 7ab2355..6c2f142 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); const char *error_get_progname(void); extern bool enable_timestamp_msg; +/* Report message and exit with error */ +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); +/* Report message with caller location and abort */ +#define error_vreport_abort(fmt, ap) \ + do { \ + error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \ + error_vreport_abort_internal(fmt, ap); \ + } while (0) +#define error_report_abort(fmt, ...) \ + do { \ + error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \ + error_report_abort_internal(fmt, ##__VA_ARGS__); \ + } while (0) + +void error_report_abort_caller_internal(const char *file, int line, const char *func); +void QEMU_NORETURN error_vreport_abort_internal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); +void QEMU_NORETURN error_report_abort_internal(const char *fmt, ...) GCC_FMT_ATTR(1, 2); + #endif diff --git a/util/qemu-error.c b/util/qemu-error.c index ecf5708..3de002b 100644 --- a/util/qemu-error.c +++ b/util/qemu-error.c @@ -237,3 +237,36 @@ void error_report(const char *fmt, ...) error_vreport(fmt, ap); va_end(ap); } + +void error_vreport_fatal(const char *fmt, va_list ap) +{ + error_vreport(fmt, ap); + exit(1); +} + +void error_report_fatal(const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + error_vreport_fatal(fmt, ap); + va_end(ap); +} + +void error_report_abort_caller_internal(const char *file, int line, const char *func) +{ + error_report("Unexpected error in %s() at %s:%d:", func, file, line); +} + +void error_vreport_abort_internal(const char *fmt, va_list ap) +{ + error_vreport(fmt, ap); + abort(); +} + +void error_report_abort_internal(const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + error_vreport_abort_internal(fmt, ap); + va_end(ap); +}
Provide two lean functions to report error messages that fatal/abort QEMU. Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- include/qemu/error-report.h | 19 +++++++++++++++++++ util/qemu-error.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)