[Qemu-devel] [PATCH 0/2] error: Documentation updates

[Qemu-devel] [PATCH 0/2] error: Documentation updates

[Markus Armbruster]

This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
Improve and document error reporting".  Lluís, feel free to integrate
my patches into a respin of your series.  You can also respin on top
of my series.

Markus Armbruster (2):
  error: Improve documentation some more
  HACKING: Add a section on error handling and reporting

 HACKING              | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/qapi/error.h | 15 ++++++++++----
 2 files changed, 66 insertions(+), 4 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] error: Improve documentation some more

[Markus Armbruster]

Don't claim error_report_err() always reports to stderr.  It actually
reports to the current monitor when we have one.

Clarify intended use of error_abort and error_fatal.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 45d6c72..e64fe54 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -27,11 +27,11 @@
  *     error_setg(&err, "invalid quark\n"
  *                "Valid quarks are up, down, strange, charm, top, bottom.");
  *
- * Report an error to stderr:
+ * Report an error to the current monitor if we have one, else stderr:
  *     error_report_err(err);
  * This frees the error object.
  *
- * Report an error to stderr with additional text prepended:
+ * Likewise, but with additional text prepended:
  *     error_reportf_err(err, "Could not frobnicate '%s': ", name);
  *
  * Report an error somewhere else:
@@ -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;
 
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] HACKING: Add a section on error handling and reporting

[Markus Armbruster]

Inspired by an RFC PATCH from Lluís Vilanova.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 HACKING | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/HACKING b/HACKING
index 12fbc8a..058aa8f 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,58 @@ 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 handling and reporting
+
+7.1 Reporting errors to the human user
+
+Do not use printf(), fprintf() or monitor_printf().  Instead, use
+error_report() or error_vreport() from error-report.h.  This ensures the
+error is reported in the right place (current monitor or stderr), and in
+a uniform format.
+
+Use error_printf() & friends to print additional information.
+
+error_report() prints the current location.  In certain common cases
+like command line parsing, the current location is tracked
+automatically.  To manipulate it manually, use the loc_*() from
+error-report.h.
+
+7.2 Propagating errors
+
+An error can't always be reported to the user right where it's detected,
+but often needs to be propagated up the call chain to a place that can
+handle it.  This can be done in various ways.
+
+The most flexible one is Error objects.  See error.h for usage
+information.
+
+Use the simplest suitable method to communicate success / failure to
+callers.  Stick to common methods: non-negative on success / -1 on
+error, non-negative / -errno, non-null / null, or Error objects.
+
+Example: when a function returns a non-null pointer on success, and it
+can fail only in one way (as far as the caller is concerned), returning
+null on failure is just fine, and certainly simpler and a lot easier on
+the eyes than propagating an Error object through an Error ** parameter.
+
+Example: when a function's callers need to report details on failure
+only the function really knows, use Error **, and set suitable errors.
+
+Do not report an error to the user when you're also returning an error
+for somebody else to handle.  Leave the reporting to the place that
+consumes the error returned.
+
+7.3 Handling errors
+
+Calling exit() is fine when handling configuration errors during
+startup.  It's problematic during normal operation.  In particular,
+monitor commands should never exit().
+
+Do not call exit() or abort() to handle an error that can be triggered
+by the guest (e.g., some unimplemented corner case in guest code
+translation or device emulation).  Guests should not be able to
+terminate QEMU.
+
+Note that &error_fatal is just another way to exit(1), and &error_abort
+is just another way to abort().
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Lluís Vilanova]

Markus Armbruster writes:

> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
> Improve and document error reporting".  Lluís, feel free to integrate
> my patches into a respin of your series.  You can also respin on top
> of my series.

Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>

I'm happy with this series as a replacement for mine. Two nitpicks:

* The error.h comments point to assert() instead of abort() as a replacement for
  error_setg(&error_abort) (while your HACKING says otherwise).

* HACKING does not explicitly point out that exit(1) is the preferred exit code.


Thanks,
  Lluis

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>> Improve and document error reporting".  Lluís, feel free to integrate
>> my patches into a respin of your series.  You can also respin on top
>> of my series.
>
> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
>
> I'm happy with this series as a replacement for mine. Two nitpicks:
>
> * The error.h comments point to assert() instead of abort() as a replacement for
>   error_setg(&error_abort) (while your HACKING says otherwise).

Where?

> * HACKING does not explicitly point out that exit(1) is the preferred exit code.

Does it need saying?  We don't seem to have a weird exit() problem in
new code.

The lower HACKING's signal-to-noise ratio gets, the fewer people will
actually read it attentively.

Thanks!

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>> 
>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>> Improve and document error reporting".  Lluís, feel free to integrate
>>> my patches into a respin of your series.  You can also respin on top
>>> of my series.
>> 
>> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> 
>> I'm happy with this series as a replacement for mine. Two nitpicks:
>> 
>> * The error.h comments point to assert() instead of abort() as a replacement for
>> error_setg(&error_abort) (while your HACKING says otherwise).

> Where?

Documentation on error_setg() (last phrase) vs HACKING (also last phrase).


>> * HACKING does not explicitly point out that exit(1) is the preferred exit code.

> Does it need saying?  We don't seem to have a weird exit() problem in
> new code.

> The lower HACKING's signal-to-noise ratio gets, the fewer people will
> actually read it attentively.

Fine by me.


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Markus Armbruster writes:
>>> 
>>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>>> Improve and document error reporting".  Lluís, feel free to integrate
>>>> my patches into a respin of your series.  You can also respin on top
>>>> of my series.
>>> 
>>> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>> 
>>> I'm happy with this series as a replacement for mine. Two nitpicks:
>>> 
>>> * The error.h comments point to assert() instead of abort() as a replacement for
>>> error_setg(&error_abort) (while your HACKING says otherwise).
>
>> Where?
>
> Documentation on error_setg() (last phrase) vs HACKING (also last phrase).

Got it.  error_setg()'s comment:

    Likewise, don't error_setg(&error_abort, ...), use assert().

This is advice on what to do.

HACKING:

    Note that &error_fatal is just another way to exit(1), and
    &error_abort is just another way to abort().

This tries to extend the admonition on exit() and abort() to
&error_fatal and &error_abort.  I'm open for better ways to word it.
However, I feel this section of HACKING should not go into detail on
what to do, but leave that to error.h.

>>> * HACKING does not explicitly point out that exit(1) is the preferred exit code.
>
>> Does it need saying?  We don't seem to have a weird exit() problem in
>> new code.
>
>> The lower HACKING's signal-to-noise ratio gets, the fewer people will
>> actually read it attentively.
>
> Fine by me.

Thanks!

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>> 
>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>> Markus Armbruster writes:
>>>> 
>>>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>>>> Improve and document error reporting".  Lluís, feel free to integrate
>>>>> my patches into a respin of your series.  You can also respin on top
>>>>> of my series.
>>>> 
>>>> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> 
>>>> I'm happy with this series as a replacement for mine. Two nitpicks:
>>>> 
>>>> * The error.h comments point to assert() instead of abort() as a replacement for
>>>> error_setg(&error_abort) (while your HACKING says otherwise).
>> 
>>> Where?
>> 
>> Documentation on error_setg() (last phrase) vs HACKING (also last phrase).

> Got it.  error_setg()'s comment:

>     Likewise, don't error_setg(&error_abort, ...), use assert().

> This is advice on what to do.

> HACKING:

>     Note that &error_fatal is just another way to exit(1), and
>     &error_abort is just another way to abort().

> This tries to extend the admonition on exit() and abort() to
> &error_fatal and &error_abort.  I'm open for better ways to word it.
> However, I feel this section of HACKING should not go into detail on
> what to do, but leave that to error.h.

Right. I just wanted to say the error.h comment should be:

     Likewise, don't error_setg(&error_abort, ...), use abort().

To be consistent with HACKING (which implies the equivalent for error_abort is
abort()). But that's just me thinking that assert will be omitted when NDEBUG is
defined :)

As a side note (just an observation), it seems that NDEBUG is only selectively
(and manually) defined in some files like "tcg/tcg.c", so it's not consistently
affecting files (e.g., it's used as a shorter substitute of "#ifdef
CONFIG_DEBUG_TCG", but it's not acting like a "real" NDEBUG; aka assert will
still work *iff* it's included before defining NDEBUG, which is not clear at
all).


>>>> * HACKING does not explicitly point out that exit(1) is the preferred exit code.
>> 
>>> Does it need saying?  We don't seem to have a weird exit() problem in
>>> new code.
>> 
>>> The lower HACKING's signal-to-noise ratio gets, the fewer people will
>>> actually read it attentively.
>> 
>> Fine by me.

> Thanks!

Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Markus Armbruster writes:
>>> 
>>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>>> Markus Armbruster writes:
>>>>> 
>>>>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>>>>> Improve and document error reporting".  Lluís, feel free to integrate
>>>>>> my patches into a respin of your series.  You can also respin on top
>>>>>> of my series.
>>>>> 
>>>>> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>>> 
>>>>> I'm happy with this series as a replacement for mine. Two nitpicks:
>>>>> 
>>>>> * The error.h comments point to assert() instead of abort() as a replacement for
>>>>> error_setg(&error_abort) (while your HACKING says otherwise).
>>> 
>>>> Where?
>>> 
>>> Documentation on error_setg() (last phrase) vs HACKING (also last phrase).
>
>> Got it.  error_setg()'s comment:
>
>>     Likewise, don't error_setg(&error_abort, ...), use assert().
>
>> This is advice on what to do.
>
>> HACKING:
>
>>     Note that &error_fatal is just another way to exit(1), and
>>     &error_abort is just another way to abort().
>
>> This tries to extend the admonition on exit() and abort() to
>> &error_fatal and &error_abort.  I'm open for better ways to word it.
>> However, I feel this section of HACKING should not go into detail on
>> what to do, but leave that to error.h.
>
> Right. I just wanted to say the error.h comment should be:
>
>      Likewise, don't error_setg(&error_abort, ...), use abort().
>
> To be consistent with HACKING (which implies the equivalent for error_abort is
> abort()). But that's just me thinking that assert will be omitted when NDEBUG is
> defined :)

In my opinion, the error.h comment should point to assert(), not
abort(), because &error_abort and assert() both print information on the
error location, while abort() doesn't.  I *want* people to use assert().

But I'd rather not explain all that in HACKING, to avoid getting bogged
down in detail there.  Feel free to suggest a wording that improves
consistency without getting too far into the weeds.

HACKING doesn't say &error_abort is *equivalent* to abort(), it says
"&error_abort is just another way to abort()".  That's true for
assert(), too.  Except assert() comes with a compile time switch we
don't use.

Heh, just found this gem in hw/virtio/virtio.c:

    #ifdef NDEBUG
    #error building with NDEBUG is not supported
    #endif

At least it got a suitable TODO comment.

> As a side note (just an observation), it seems that NDEBUG is only selectively
> (and manually) defined in some files like "tcg/tcg.c", so it's not consistently
> affecting files (e.g., it's used as a shorter substitute of "#ifdef
> CONFIG_DEBUG_TCG", but it's not acting like a "real" NDEBUG; aka assert will
> still work *iff* it's included before defining NDEBUG, which is not clear at
> all).

I can see two instances (tci.c tcg/tcg.c), and both look like this:

    #if !defined(CONFIG_DEBUG_TCG) && !defined(NDEBUG)
    # define NDEBUG
    #endif

They make switching off CONFIG_DEBUG_TCG switch off assertions as well.
Feels odd to me, but I'm not familiar with these two files.

[...]

Re: [Qemu-devel] [PATCH 0/2] error: Documentation updates

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>> 
>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>> Markus Armbruster writes:
>>>> 
>>>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>>>> Markus Armbruster writes:
>>>>>> 
>>>>>>> This overlaps with parts of Lluís's "[RFC][PATCH v6 0/5] utils:
>>>>>>> Improve and document error reporting".  Lluís, feel free to integrate
>>>>>>> my patches into a respin of your series.  You can also respin on top
>>>>>>> of my series.
>>>>>> 
>>>>>> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>>>> 
>>>>>> I'm happy with this series as a replacement for mine. Two nitpicks:
>>>>>> 
>>>>>> * The error.h comments point to assert() instead of abort() as a replacement for
>>>>>> error_setg(&error_abort) (while your HACKING says otherwise).
>>>> 
>>>>> Where?
>>>> 
>>>> Documentation on error_setg() (last phrase) vs HACKING (also last phrase).
>> 
>>> Got it.  error_setg()'s comment:
>> 
>>> Likewise, don't error_setg(&error_abort, ...), use assert().
>> 
>>> This is advice on what to do.
>> 
>>> HACKING:
>> 
>>> Note that &error_fatal is just another way to exit(1), and
>>> &error_abort is just another way to abort().
>> 
>>> This tries to extend the admonition on exit() and abort() to
>>> &error_fatal and &error_abort.  I'm open for better ways to word it.
>>> However, I feel this section of HACKING should not go into detail on
>>> what to do, but leave that to error.h.
>> 
>> Right. I just wanted to say the error.h comment should be:
>> 
>> Likewise, don't error_setg(&error_abort, ...), use abort().
>> 
>> To be consistent with HACKING (which implies the equivalent for error_abort is
>> abort()). But that's just me thinking that assert will be omitted when NDEBUG is
>> defined :)

> In my opinion, the error.h comment should point to assert(), not
> abort(), because &error_abort and assert() both print information on the
> error location, while abort() doesn't.  I *want* people to use assert().

> But I'd rather not explain all that in HACKING, to avoid getting bogged
> down in detail there.  Feel free to suggest a wording that improves
> consistency without getting too far into the weeds.

> HACKING doesn't say &error_abort is *equivalent* to abort(), it says
> "&error_abort is just another way to abort()".  That's true for
> assert(), too.  Except assert() comes with a compile time switch we
> don't use.

Well, now that I think of it, assert() uses abort() underneath, so they're
equivalent on that sense (not that I thought of it initially).

HACKING:

    Note that &error_fatal is just another way to exit(1), and &error_abort is
    just another way to terminate QEMU like abort() or assert().

Also, are we all aware that messages from abort() won't be redirected to the
monitor? (error_setg, error_report and so redirect appropriately). Just checking
if it makes sense to redirect them.

BTW, either way, I'm fine if you push this into upstream.


[...]
>> As a side note (just an observation), it seems that NDEBUG is only selectively
>> (and manually) defined in some files like "tcg/tcg.c", so it's not consistently
>> affecting files (e.g., it's used as a shorter substitute of "#ifdef
>> CONFIG_DEBUG_TCG", but it's not acting like a "real" NDEBUG; aka assert will
>> still work *iff* it's included before defining NDEBUG, which is not clear at
>> all).

> I can see two instances (tci.c tcg/tcg.c), and both look like this:

>     #if !defined(CONFIG_DEBUG_TCG) && !defined(NDEBUG)
>     # define NDEBUG
>     #endif

> They make switching off CONFIG_DEBUG_TCG switch off assertions as well.
> Feels odd to me, but I'm not familiar with these two files.

That's the case now, since assert.h is never directly (or indirectly) included
before the define. But it looks fragile to me; it's very easy to add an include
before that define.


Cheers,
  Lluis