utils: Improve and document error reporting

Provide two lean functions to report error messages that fatal/abort QEMU.

Signed-off-by Lluís Vilanova <[email protected]>

---
 include/qemu/error-report.h |   19 +++++++++++++++++++
 util/qemu-error.c           |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

include/qemu/error-report.h +0 -0

index 7ab2355..6c2f142 100644

include/qemu/error-report.h +7 -0

+/* 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, ...)                                    \

util/qemu-error.c +8 -0

index ecf5708..3de002b 100644
@@ -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, ...)

util/qemu-error.c +15 -0

+}
+
+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);
+}

Lluís Vilanova <[email protected]> writes:

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 26 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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);

Quoted 1 line

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

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 4 lines

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, ...).

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 57 lines · Markus Armbruster

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 1 line

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?

Quoted 58 lines · Thomas Huth

Quoted 57 lines · Markus Armbruster

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 4 lines

Quoted 1 line

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?

I feel such detailed advice belings into error.h. Sketch appended.

If that doesn't succeed in keeping (c) out, make checkpatch flag it.

Quoted 3 lines

(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).

Quoted 4 lines

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?

As I wrote in my first reply to this thread, I'd like them to be cleaned up to just abort() or assert().

I like assert(), because it gives me exactly what I can use to debug the programming error: a core dump (if enabled) and a source location (useful when no core dump). I never bought the argument that we should use abort() instead of assert(0) because "what if NDEBUG?!?". If you define NDEBUG, our 600+ abort()s won't save you from our 4000+ assert()s.

include/qapi/error.h +6 -1

index 45d6c72..ea7e74f 100644
@@ -162,6 +162,9 @@ ErrorClass error_get_class(const Error *err);
  * human-readable error message is made from printf-style @fmt, ...
  * The resulting message should be a single phrase, with no newline or
  * trailing punctuation.
+ * Please don't error_setg(&error_fatal, ...), use error_report() and
+ * exit(), because that's more obvious.
+ * Likewise, don't error_setg(&error_abort, ...), use assert().
  */
 #define error_setg(errp, fmt, ...)                              \
     error_setg_internal((errp), __FILE__, __LINE__, __func__,   \

@@ -213,6 +216,8 @@ void error_setg_win32_internal(Error **errp,
  * the error object.
  * Else, move the error object from @local_err to *@dst_errp.
  * On return, @local_err is invalid.
+ * Please don't error_propagate(&error_fatal, ...), use
+ * error_report_err() and exit(), because that's more obvious.
  */
 void error_propagate(Error **dst_errp, Error *local_err);
 

@@ -291,12 +296,14 @@ void error_set_internal(Error **errp,
     GCC_FMT_ATTR(6, 7);
 
 /*
- * Pass to error_setg() & friends to abort() on error.
+ * Special error destination to abort on error.

Quoted 59 lines · Markus Armbruster

Quoted 58 lines · Thomas Huth

Quoted 57 lines · Markus Armbruster

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

Quoted 4 lines

Quoted 1 line

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?

I feel such detailed advice belings into error.h. Sketch appended.

If that doesn't succeed in keeping (c) out, make checkpatch flag it.

Quoted 3 lines

(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).

I can easily change that, no problem. I'm just happy consensus is landing on this subject.

Quoted 8 lines

Quoted 4 lines

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?

As I wrote in my first reply to this thread, I'd like them to be cleaned up to just abort() or assert().

Quoted 6 lines

I like assert(), because it gives me exactly what I can use to debug the programming error: a core dump (if enabled) and a source location (useful when no core dump). I never bought the argument that we should use abort() instead of assert(0) because "what if NDEBUG?!?". If you define NDEBUG, our 600+ abort()s won't save you from our 4000+ assert()s.

Sorry, but I don't buy the argument of, "I prefer assert() because there's already lots of them". To me, there's a semantic difference between debug builds and regular ones (aka, assert vs abort).

Also, I think it adds to the confusion
that assert and abort seem to be used interchangeably in the code.

What about this definition?

* exit(): user-triggered errors
* abort(): general programming errors
* assert(): additional sanity/consistency checks against programming errors

Now, abort & assert have an overlap. Should we discourage one in favour of the other?

Also:

* error_report_fatal ensures the same exit code is always used (otherwise it can fail with inconsistent error codes)

* error_report_abort brings the code information of assert into abort

But of course, I'm happy either way :)

Quoted 29 lines

include/qapi/error.h +6 -1

index 45d6c72..ea7e74f 100644
@@ -162,6 +162,9 @@ ErrorClass error_get_class(const Error *err);
  * human-readable error message is made from printf-style @fmt, ...
  * The resulting message should be a single phrase, with no newline or
  * trailing punctuation.
+ * Please don't error_setg(&error_fatal, ...), use error_report() and
+ * exit(), because that's more obvious.
+ * Likewise, don't error_setg(&error_abort, ...), use assert().
  */
 #define error_setg(errp, fmt, ...)                              \
     error_setg_internal((errp), __FILE__, __LINE__, __func__,   \

@@ -213,6 +216,8 @@ void error_setg_win32_internal(Error **errp,
  * the error object.
  * Else, move the error object from @local_err to *@dst_errp.
  * On return, @local_err is invalid.
+ * Please don't error_propagate(&error_fatal, ...), use
+ * error_report_err() and exit(), because that's more obvious.
  */
 void error_propagate(Error **dst_errp, Error *local_err);
 

@@ -291,12 +296,14 @@ void error_set_internal(Error **errp,
     GCC_FMT_ATTR(6, 7);
 
 /*
- * Pass to error_setg() & friends to abort() on error.
+ * Special error destination to abort on error.

include/qapi/error.h void error_set_internal(Error **errp, · line 308

Quoted 10 lines

include/qapi/error.h +3 -1

+ * See error_setg() and error_propagate() for details.
  */
 extern Error *error_abort;
 
 /*
- * Pass to error_setg() & friends to exit(1) on error.
+ * Special error destination to exit(1) on error.
+ * See error_setg() and error_propagate() for details.
  */
 extern Error *error_fatal;

I see, this will make it clearer for people looking for functions without reading HACKING. I can add this and reference it from the document.

Quoted 93 lines · Lluís Vilanova

Quoted 59 lines · Markus Armbruster

Quoted 58 lines · Thomas Huth

Quoted 57 lines · Markus Armbruster

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

Quoted 4 lines

Quoted 1 line

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?

I feel such detailed advice belings into error.h. Sketch appended.

If that doesn't succeed in keeping (c) out, make checkpatch flag it.

Quoted 3 lines

(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).

I can easily change that, no problem. I'm just happy consensus is landing on this subject.

Quoted 8 lines

Quoted 4 lines

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?

As I wrote in my first reply to this thread, I'd like them to be cleaned up to just abort() or assert().

Quoted 6 lines

I like assert(), because it gives me exactly what I can use to debug the programming error: a core dump (if enabled) and a source location (useful when no core dump). I never bought the argument that we should use abort() instead of assert(0) because "what if NDEBUG?!?". If you define NDEBUG, our 600+ abort()s won't save you from our 4000+ assert()s.

Quoted 3 lines

Sorry, but I don't buy the argument of, "I prefer assert() because there's already lots of them". To me, there's a semantic difference between debug builds and regular ones (aka, assert vs abort).

That's not what I said :)

In the past, people have argued in favor of abort() by pointing to NDEBUG. I don't buy that argument, but me not buying it is not why I prefer assert(). I do because it prints additional information that's occasionally useful.

Quoted 2 lines

Also, I think it adds to the confusion
that assert and abort seem to be used interchangeably in the code.

For better or worse, we overwhelmingly use abort() instead of assert(0), but don't use if (!good) abort() instead of assert(good). Doesn't make sense to me, but my appetite for tree-wide changes and the debates that go with them has limits.

Quoted 8 lines

What about this definition?

* exit(): user-triggered errors
* abort(): general programming errors
* assert(): additional sanity/consistency checks against programming errors

Now, abort & assert have an overlap. Should we discourage one in favour of the other?

I can't see how to decide whether a programming error is "general" or "additional", or why an "additional" one error deserves a message pointing to source code, but a "general" one does not.

Quoted 4 lines

Also:

* error_report_fatal ensures the same exit code is always used (otherwise it can fail with inconsistent error codes)

What if you *want* to use a different exit code?

But I grant you that we should almost always use exit(1) for fatal errors. And in fact we do! There are a bunch of misguided exit(-1) in the code, but git-log -S'exit(-1)' finds only half a dozen offending commits since 2013, and none since 2015, so preventing more seems to be a mostly solved problem.

Quoted 1 line

* error_report_abort brings the code information of assert into abort

If you want your crashes to print source location information, don't reinvent the wheel, just use assert().

&error_abort can't because the interesting spot isn't where we decide to abort, but where the error got created.

Quoted 43 lines

But of course, I'm happy either way :)

Quoted 39 lines

include/qapi/error.h +9 -2

index 45d6c72..ea7e74f 100644
@@ -162,6 +162,9 @@ ErrorClass error_get_class(const Error *err);
 * human-readable error message is made from printf-style @fmt, ...
 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.
+ * Please don't error_setg(&error_fatal, ...), use error_report() and
+ * exit(), because that's more obvious.
+ * Likewise, don't error_setg(&error_abort, ...), use assert().
 */
 #define error_setg(errp, fmt, ...)                              \
 error_setg_internal((errp), __FILE__, __LINE__, __func__,   \

@@ -213,6 +216,8 @@ void error_setg_win32_internal(Error **errp,
 * the error object.
 * Else, move the error object from @local_err to *@dst_errp.
 * On return, @local_err is invalid.
+ * Please don't error_propagate(&error_fatal, ...), use
+ * error_report_err() and exit(), because that's more obvious.
 */
 void error_propagate(Error **dst_errp, Error *local_err);
 

@@ -291,12 +296,14 @@ void error_set_internal(Error **errp,
 GCC_FMT_ATTR(6, 7);
 
 /*
- * Pass to error_setg() & friends to abort() on error.
+ * Special error destination to abort on error.
+ * See error_setg() and error_propagate() for details.
 */
 extern Error *error_abort;
 
 /*
- * Pass to error_setg() & friends to exit(1) on error.
+ * Special error destination to exit(1) on error.
+ * See error_setg() and error_propagate() for details.
 */
 extern Error *error_fatal;

Quoted 2 lines

I see, this will make it clearer for people looking for functions without reading HACKING. I can add this and reference it from the document.

If you like, I can post it as a formal patch you can then include in your series.

Quoted 140 lines · Markus Armbruster

Quoted 93 lines · Lluís Vilanova

Quoted 59 lines · Markus Armbruster

Quoted 58 lines · Thomas Huth

Quoted 57 lines · Markus Armbruster

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

Quoted 4 lines

Quoted 1 line

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?

I feel such detailed advice belings into error.h. Sketch appended.

If that doesn't succeed in keeping (c) out, make checkpatch flag it.

Quoted 3 lines

(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).

I can easily change that, no problem. I'm just happy consensus is landing on this subject.

Quoted 8 lines

Quoted 4 lines

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?

As I wrote in my first reply to this thread, I'd like them to be cleaned up to just abort() or assert().

Quoted 6 lines

I like assert(), because it gives me exactly what I can use to debug the programming error: a core dump (if enabled) and a source location (useful when no core dump). I never bought the argument that we should use abort() instead of assert(0) because "what if NDEBUG?!?". If you define NDEBUG, our 600+ abort()s won't save you from our 4000+ assert()s.

Quoted 3 lines

Sorry, but I don't buy the argument of, "I prefer assert() because there's already lots of them". To me, there's a semantic difference between debug builds and regular ones (aka, assert vs abort).

That's not what I said :)

In the past, people have argued in favor of abort() by pointing to NDEBUG. I don't buy that argument, but me not buying it is not why I prefer assert(). I do because it prints additional information that's occasionally useful.

Quoted 2 lines

Also, I think it adds to the confusion
that assert and abort seem to be used interchangeably in the code.

For better or worse, we overwhelmingly use abort() instead of assert(0), but don't use if (!good) abort() instead of assert(good). Doesn't make sense to me, but my appetite for tree-wide changes and the debates that go with them has limits.

Quoted 8 lines

What about this definition?

* exit(): user-triggered errors
* abort(): general programming errors
* assert(): additional sanity/consistency checks against programming errors

Now, abort & assert have an overlap. Should we discourage one in favour of the other?

I can't see how to decide whether a programming error is "general" or "additional", or why an "additional" one error deserves a message pointing to source code, but a "general" one does not.

Quoted 4 lines

Also:

* error_report_fatal ensures the same exit code is always used (otherwise it can fail with inconsistent error codes)

What if you *want* to use a different exit code?

But I grant you that we should almost always use exit(1) for fatal errors. And in fact we do! There are a bunch of misguided exit(-1) in the code, but git-log -S'exit(-1)' finds only half a dozen offending commits since 2013, and none since 2015, so preventing more seems to be a mostly solved problem.

Quoted 1 line

* error_report_abort brings the code information of assert into abort

Quoted 5 lines

If you want your crashes to print source location information, don't reinvent the wheel, just use assert().

&error_abort can't because the interesting spot isn't where we decide to abort, but where the error got created.

Fair enough. I don't want a flame on style either, although I might look like wanting one :)

Quoted 43 lines

Quoted 43 lines

But of course, I'm happy either way :)

Quoted 39 lines

include/qapi/error.h +9 -2

index 45d6c72..ea7e74f 100644
@@ -162,6 +162,9 @@ ErrorClass error_get_class(const Error *err);
 * human-readable error message is made from printf-style @fmt, ...
 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.
+ * Please don't error_setg(&error_fatal, ...), use error_report() and
+ * exit(), because that's more obvious.
+ * Likewise, don't error_setg(&error_abort, ...), use assert().
 */
 #define error_setg(errp, fmt, ...)                              \
 error_setg_internal((errp), __FILE__, __LINE__, __func__,   \

@@ -213,6 +216,8 @@ void error_setg_win32_internal(Error **errp,
 * the error object.
 * Else, move the error object from @local_err to *@dst_errp.
 * On return, @local_err is invalid.
+ * Please don't error_propagate(&error_fatal, ...), use
+ * error_report_err() and exit(), because that's more obvious.
 */
 void error_propagate(Error **dst_errp, Error *local_err);
 

@@ -291,12 +296,14 @@ void error_set_internal(Error **errp,
 GCC_FMT_ATTR(6, 7);
 
 /*
- * Pass to error_setg() & friends to abort() on error.
+ * Special error destination to abort on error.
+ * See error_setg() and error_propagate() for details.
 */
 extern Error *error_abort;
 
 /*
- * Pass to error_setg() & friends to exit(1) on error.
+ * Special error destination to exit(1) on error.
+ * See error_setg() and error_propagate() for details.
 */
 extern Error *error_fatal;

Quoted 5 lines

Quoted 2 lines

I see, this will make it clearer for people looking for functions without reading HACKING. I can add this and reference it from the document.

If you like, I can post it as a formal patch you can then include in your series.

That'd be great. Please cc me when you send it.

Quoted 33 lines · David Gibson

Quoted 32 lines · Thomas Huth

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 27 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 1 line · Markus Armbruster

Lluís Vilanova <[email protected]> writes:

...

Quoted 21 lines

include/qemu/error-report.h void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); · line 48

Quoted 11 lines

include/qemu/error-report.h +3 -0

index 7ab2355..6c2f142 100644
@@ -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);

Quoted 6 lines

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?

As to the " " messages: surely you're joking, Mr. Feynman :)

Replaces all direct uses of 'error_setg(&error_fatal/abort)' with 'error_report_fatal/abort'. Also reimplements the former on top of the latter.

Signed-off-by Lluís Vilanova <[email protected]>

---
 hw/block/fdc.c     |    6 +++---
 hw/ppc/spapr.c     |    8 ++++----
 hw/ppc/spapr_drc.c |    2 +-
 util/error.c       |    9 +++------
 4 files changed, 11 insertions(+), 14 deletions(-)

hw/block/fdc.c +0 -0

index e3b0e1e..8f0c947 100644
@@ -347,9 +347,9 @@ static int pick_geometry(FDrive *drv)
 

hw/block/fdc.c +0 -0

     parse = &(fd_formats[match]);

hw/ppc/spapr.c +4 -4

index 5bd8fd3..3c0f339 100644
@@ -1039,7 +1039,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
          * For HV KVM, host kernel will return -ENOMEM when requested
          * HTAB size can't be allocated.
          */
-        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
+        error_report_abort("Failed to allocate HTAB of requested size, try with smaller maxmem");
     } else if (shift > 0) {
         /*
          * Kernel handles htab, we don't need to allocate one

@@ -1048,7 +1048,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
          * but we don't allow booting of such guests.
          */
         if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
+            error_report_abort("Failed to allocate HTAB of requested size, try with smaller maxmem");
         }
 
         spapr->htab_shift = shift;

@@ -1079,10 +1079,10 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
 
     shift = kvmppc_reset_htab(spapr->htab_shift);
     if (shift < 0) {
-        error_setg(&error_abort, "Failed to reset HTAB");
+        error_report_abort("Failed to reset HTAB");
     } else if (shift > 0) {
         if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Requested HTAB allocation failed during reset");
+            error_report_abort("Requested HTAB allocation failed during reset");
         }
 
         /* Tell readers to update their file descriptor */

hw/ppc/spapr_drc.c +1 -1

index 90016e6..2228124 100644
@@ -323,7 +323,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque,
             break;
         }
         default:
-            error_setg(&error_abort, "device FDT in unexpected state: %d", tag);
+            error_report_abort("device FDT in unexpected state: %d", tag);
         }
         fdt_offset = fdt_offset_next;
     } while (fdt_depth != 0);

util/error.c +3 -6

index 57303fd..b8a9120 100644
@@ -30,15 +30,12 @@ Error *error_fatal;
 
 static void error_handle_fatal(Error **errp, Error *err)
 {
+    /* None of them has a hint, so error_report_err() is not necessary here */
     if (errp == &error_abort) {
-        fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
-                err->func, err->src, err->line);
-        error_report_err(err);
-        abort();
+        error_report_abort_internal("%s", err->msg);
     }
     if (errp == &error_fatal) {
-        error_report_err(err);
-        exit(1);
+        error_report_fatal("%s", err->msg);
     }
 }
 

Signed-off-by Lluís Vilanova <[email protected]>

---
 target-ppc/kvm.c        |    4 ++--
 target-ppc/kvm_ppc.h    |   15 +++++++++------
 target-ppc/mmu-hash32.c |    5 +++--
 target-ppc/mmu_helper.c |    3 +--
 4 files changed, 15 insertions(+), 12 deletions(-)

Signed-off-by Lluís Vilanova <[email protected]>

---
 target-ppc/kvm.c        |    4 ++--
 target-ppc/kvm_ppc.h    |   15 +++++++++------
 target-ppc/mmu-hash32.c |    5 +++--
 target-ppc/mmu_helper.c |    3 +--
 4 files changed, 15 insertions(+), 12 deletions(-)

target-ppc/kvm.c +2 -2

index 1bb87e6..b743811 100644
@@ -587,7 +587,7 @@ static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
 
         default:
             /* Don't handle this size yet */
-            abort();
+            error_report_abort("Unhandled size: %d", id & KVM_REG_SIZE_MASK);
         }
     }
 }

@@ -617,7 +617,7 @@ static void kvm_put_one_spr(CPUState *cs, uint64_t id, int spr)
 
     default:
         /* Don't handle this size yet */
-        abort();
+        error_report_abort("Unhandled size: %d", id & KVM_REG_SIZE_MASK);
     }
 
     ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);

target-ppc/kvm_ppc.h +3 -0

index 62406ce..f82582e 100644
@@ -9,6 +9,9 @@
 #ifndef __KVM_PPC_H__
 #define __KVM_PPC_H__
 
+#include "qemu/error-report.h"
+
+
 #define TYPE_HOST_POWERPC_CPU "host-" TYPE_POWERPC_CPU
 
 #ifdef CONFIG_KVM

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by Lluís Vilanova <[email protected]>

---
 HACKING |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

HACKING +0 -0

index 12fbc8a..b738bce 100644

Quoted 12 lines · Lluís Vilanova

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by Lluís Vilanova <[email protected]>

---
HACKING | 37 +++++++++++++++++++++++++++++++++++++

1 file changed, 37 insertions(+)

HACKING +0 -0

index 12fbc8a..b738bce 100644

HACKING painful. These are: · line 166

Quoted 10 lines

HACKING +7 -0

@@ -157,3 +157,40 @@ painful. These are:
 * you may assume that integers are 2s complement representation
 * you may assume that right shift of a signed integer duplicates
 the sign bit (ie it is an arithmetic shift, not a logical shift)
+
+7. Error reporting
+
+QEMU provides various mechanisms for reporting errors using a uniform format,
+ensuring the user will receive them (e.g., shown in QMP when necessary). You
+should use one of these mechanisms instead of manually reporting them (i.e., do
+not use 'printf()', 'exit()' or 'abort()').

The "do not use exit() or abort()" part applies only if we adopt your new error reporting functions.

HACKING painful. These are: · line 168

Quoted 2 lines

HACKING +2 -0

+
+As a general rule, use the 'fatal' forms below for errors that can be triggered

Where's "below"?

HACKING painful. These are: · line 170

Quoted 2 lines

HACKING +2 -0

+by the user (e.g., commandline, hotplug, etc.), and the 'abort' forms for
+programming errors that the user should not be able to trigger.

This paragraph applies only if we adopt your new error reporting functions.

HACKING painful. These are: · line 175

Quoted 5 lines

HACKING +5 -0

+
+7.1. Simple error messages
+
+The 'error_report*()' functions in "include/qemu/error-report.h" will
+immediately report error messages to the user.

Suggest "the human user".

HACKING painful. These are: · line 181

Quoted 6 lines

HACKING +6 -0

+
+WARNING: Do *not* use 'error_report_fatal()' or 'error_report_abort()' for
+errors that are (or can be) triggered by guest code (e.g., some unimplemented
+corner case in guest code translation or device code). Otherwise, that can be
+abused by guest code to terminate QEMU. Instead, you should use
+'error_report()'.

This paragraph applies only if we adopt your new error reporting functions. Without them: "Do *not* exit() or abort() on errors that can be triggered...", and scratch the last sentence.

HACKING painful. These are: · line 187

Quoted 6 lines

HACKING +6 -0

+
+7.2. Errors in user inputs
+
+The 'loc_*()' functions in "include/qemu/error-report.h" will extend the
+messages from 'error_report*()' with references to locations in inputs provided
+by the user (e.g., command line arguments or configuration files).

This is probably too terse to help much on its own. Perhaps error-report.h should have usage information, like error.h.

HACKING painful. These are: · line 191

Quoted 4 lines

HACKING +4 -0

+
+7.3. More complex error management
+
+The functions in "include/qapi/error.h" can be used to accumulate error messages

Long line.

HACKING painful. These are: · line 193

Quoted 2 lines

HACKING +2 -0

+in an 'Error' object, which can be propagated up the call chain where it is
+finally reported.

"Accumulate" suggests accumulating multiple error messages, and that's not quite right. Perhaps: "to capture an error message".

"where it is finally reported" is inaccurate. It may or may not be reported. Perhaps: "until it is handled, and possibly reported to the user".

HACKING painful. These are: · line 196

Quoted 3 lines

HACKING +3 -0

+
+WARNING: The special 'error_fatal' and 'error_abort' objects follow the same
+constraints as the 'error_report_fatal()' and 'error_report_abort()' functions.

Without your new error reporting functions: "Do *not* use &error_fatal and &error_abort to handle errors that can be triggered by guest code, just like exit() and abort()."

Suggest to explicitly point to the big usage comment in error.h.

Quoted 12 lines · Markus Armbruster

Quoted 11 lines · Lluís Vilanova

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by Lluís Vilanova <[email protected]>

---
HACKING | 37 +++++++++++++++++++++++++++++++++++++

1 file changed, 37 insertions(+)

HACKING +0 -0

index 12fbc8a..b738bce 100644

Quoted 8 lines

HACKING painful. These are: · line 187

Quoted 5 lines

+7.2. Errors in user inputs

HACKING +4 -0

+
+The 'loc_*()' functions in "include/qemu/error-report.h" will extend the
+messages from 'error_report*()' with references to locations in inputs provided
+by the user (e.g., command line arguments or configuration files).

This is probably too terse to help much on its own. Perhaps error-report.h should have usage information, like error.h.

I can try adding that, although I've barely used this part of the interface.

Quoted 14 lines · Lluís Vilanova

Quoted 12 lines · Markus Armbruster

Quoted 11 lines · Lluís Vilanova

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by Lluís Vilanova <[email protected]>

---
HACKING | 37 +++++++++++++++++++++++++++++++++++++

1 file changed, 37 insertions(+)

HACKING +0 -0

index 12fbc8a..b738bce 100644

[...]

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

+7.2. Errors in user inputs

HACKING +4 -0

+
+The 'loc_*()' functions in "include/qemu/error-report.h" will extend the
+messages from 'error_report*()' with references to locations in inputs provided
+by the user (e.g., command line arguments or configuration files).

Quoted 4 lines

Quoted 2 lines

This is probably too terse to help much on its own. Perhaps error-report.h should have usage information, like error.h.

I can try adding that, although I've barely used this part of the interface.

Documenting something you're not familiar with risks messy and laborious review. I'd simply drop this section for now. If you have appetite for more after you got the rest in, you can do another patch.

Quoted 22 lines · Markus Armbruster

Quoted 14 lines · Lluís Vilanova

Quoted 12 lines · Markus Armbruster

Quoted 11 lines · Lluís Vilanova

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by Lluís Vilanova <[email protected]>

---
HACKING | 37 +++++++++++++++++++++++++++++++++++++

1 file changed, 37 insertions(+)

HACKING +0 -0

index 12fbc8a..b738bce 100644

[...]

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

+7.2. Errors in user inputs

HACKING +4 -0

+
+The 'loc_*()' functions in "include/qemu/error-report.h" will extend the
+messages from 'error_report*()' with references to locations in inputs provided
+by the user (e.g., command line arguments or configuration files).

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

This is probably too terse to help much on its own. Perhaps error-report.h should have usage information, like error.h.

I can try adding that, although I've barely used this part of the interface.

Documenting something you're not familiar with risks messy and laborious review. I'd simply drop this section for now. If you have appetite for more after you got the rest in, you can do another patch.

Mmmm, I still think that a terse reference is better at directing developers to the right header than just not commenting it. I think that this type of patches are not funny to anyone, so this risks having no metion of loc_* in the near/mid future.

But hey, I might just be too pessimistic :)

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by Lluís Vilanova <[email protected]>