* [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-02 16:13 [Qemu-devel] [RFC][PATCH v6 0/5] utils: Improve and document error reporting Lluís Vilanova
@ 2016-02-02 16:13 ` Lluís Vilanova
2016-02-02 18:53 ` Markus Armbruster
2016-02-02 16:14 ` [Qemu-devel] [PATCH v6 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort) Lluís Vilanova
` (3 subsequent siblings)
4 siblings, 1 reply; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:13 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Stefan Hajnoczi, Dr . David Alan Gilbert,
Markus Armbruster, David Gibson
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);
+/* 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);
+}
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-02 16:13 ` [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort Lluís Vilanova
@ 2016-02-02 18:53 ` Markus Armbruster
2016-02-02 21:47 ` Thomas Huth
2016-02-03 17:59 ` Lluís Vilanova
0 siblings, 2 replies; 32+ messages in thread
From: Markus Armbruster @ 2016-02-02 18:53 UTC (permalink / raw)
To: Lluís Vilanova
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
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);
> +}
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-02 18:53 ` Markus Armbruster
@ 2016-02-02 21:47 ` Thomas Huth
2016-02-03 5:04 ` David Gibson
2016-02-03 7:26 ` Markus Armbruster
2016-02-03 17:59 ` Lluís Vilanova
1 sibling, 2 replies; 32+ messages in thread
From: Thomas Huth @ 2016-02-02 21:47 UTC (permalink / raw)
To: Markus Armbruster, Lluís Vilanova
Cc: Stefan Hajnoczi, David Gibson, qemu-devel, Dr . David Alan Gilbert
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-02 21:47 ` Thomas Huth
@ 2016-02-03 5:04 ` David Gibson
2016-02-03 9:48 ` Markus Armbruster
2016-02-03 7:26 ` Markus Armbruster
1 sibling, 1 reply; 32+ messages in thread
From: David Gibson @ 2016-02-03 5:04 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Stefan Hajnoczi, Markus Armbruster,
Dr . David Alan Gilbert, Lluís Vilanova
[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]
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
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 5:04 ` David Gibson
@ 2016-02-03 9:48 ` Markus Armbruster
2016-02-03 9:58 ` Thomas Huth
2016-02-03 22:23 ` David Gibson
0 siblings, 2 replies; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 9:48 UTC (permalink / raw)
To: David Gibson
Cc: Lluís Vilanova, Stefan Hajnoczi, Thomas Huth, qemu-devel,
Dr . David Alan Gilbert
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.
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 9:48 ` Markus Armbruster
@ 2016-02-03 9:58 ` Thomas Huth
2016-02-03 10:38 ` Markus Armbruster
2016-02-03 22:23 ` David Gibson
1 sibling, 1 reply; 32+ messages in thread
From: Thomas Huth @ 2016-02-03 9:58 UTC (permalink / raw)
To: Markus Armbruster, Lluís Vilanova
Cc: Stefan Hajnoczi, qemu-devel, Dr . David Alan Gilbert, David Gibson
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 9:58 ` Thomas Huth
@ 2016-02-03 10:38 ` Markus Armbruster
2016-02-03 13:42 ` Lluís Vilanova
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 10:38 UTC (permalink / raw)
To: Thomas Huth
Cc: Stefan Hajnoczi, David Gibson, Lluís Vilanova,
Dr . David Alan Gilbert, qemu-devel
Thomas Huth <thuth@redhat.com> writes:
> 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?
I feel such detailed advice belings into error.h. Sketch appended.
If that doesn't succeed in keeping (c) out, make checkpatch flag it.
> (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?
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.
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 45d6c72..ea7e74f 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -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;
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 10:38 ` Markus Armbruster
@ 2016-02-03 13:42 ` Lluís Vilanova
2016-02-03 14:34 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-03 13:42 UTC (permalink / raw)
To: Markus Armbruster
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
Markus Armbruster writes:
> Thomas Huth <thuth@redhat.com> writes:
>> 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?
> I feel such detailed advice belings into error.h. Sketch appended.
> If that doesn't succeed in keeping (c) out, make checkpatch flag it.
>> (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.
>> 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.
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 :)
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 45d6c72..ea7e74f 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -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;
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.
Thanks,
Lluis
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 13:42 ` Lluís Vilanova
@ 2016-02-03 14:34 ` Markus Armbruster
2016-02-03 15:11 ` Lluís Vilanova
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 14:34 UTC (permalink / raw)
To: Thomas Huth
Cc: Stefan Hajnoczi, David Gibson, qemu-devel, Dr . David Alan Gilbert
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Markus Armbruster writes:
>
>> Thomas Huth <thuth@redhat.com> writes:
>>> 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?
>
>> I feel such detailed advice belings into error.h. Sketch appended.
>
>> If that doesn't succeed in keeping (c) out, make checkpatch flag it.
>
>>> (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.
>
>
>>> 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.
>
> 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.
> 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.
> 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.
> 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.
> * 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.
> But of course, I'm happy either way :)
>
>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 45d6c72..ea7e74f 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -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;
>
> 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.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 14:34 ` Markus Armbruster
@ 2016-02-03 15:11 ` Lluís Vilanova
2016-02-03 18:06 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-03 15:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: Stefan Hajnoczi, Thomas Huth, qemu-devel,
Dr . David Alan Gilbert, David Gibson
Markus Armbruster writes:
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>>
>>> Thomas Huth <thuth@redhat.com> writes:
>>>> 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?
>>
>>> I feel such detailed advice belings into error.h. Sketch appended.
>>
>>> If that doesn't succeed in keeping (c) out, make checkpatch flag it.
>>
>>>> (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.
>>
>>
>>>> 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.
>>
>> 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.
>> 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.
>> 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.
>> 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.
>> * 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.
Fair enough. I don't want a flame on style either, although I might look like
wanting one :)
>> But of course, I'm happy either way :)
>>
>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index 45d6c72..ea7e74f 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -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;
>>
>> 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.
Thanks,
Lluis
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 15:11 ` Lluís Vilanova
@ 2016-02-03 18:06 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 18:06 UTC (permalink / raw)
To: Thomas Huth
Cc: Stefan Hajnoczi, qemu-devel, Dr . David Alan Gilbert, David Gibson
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Markus Armbruster writes:
>>>
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>> 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?
>>>
>>>> I feel such detailed advice belings into error.h. Sketch appended.
>>>
>>>> If that doesn't succeed in keeping (c) out, make checkpatch flag it.
>>>
>>>>> (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.
>>>
>>>
>>>>> 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.
>>>
>>> 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.
>
>>> 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.
>
>>> 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.
>
>>> 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.
>
>>> * 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.
>
> Fair enough. I don't want a flame on style either, although I might look like
> wanting one :)
I think we're having a civil, constructive discussion on error handling
and reporting that happens to include stylistic aspects :)
>>> But of course, I'm happy either way :)
>>>
>>>
>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>> index 45d6c72..ea7e74f 100644
>>>> --- a/include/qapi/error.h
>>>> +++ b/include/qapi/error.h
>>>> @@ -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;
>>>
>>> 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.
Done: [PATCH 0/2] error: Documentation updates
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-03 9:48 ` Markus Armbruster
2016-02-03 9:58 ` Thomas Huth
@ 2016-02-03 22:23 ` David Gibson
1 sibling, 0 replies; 32+ messages in thread
From: David Gibson @ 2016-02-03 22:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: Lluís Vilanova, Stefan Hajnoczi, Thomas Huth, qemu-devel,
Dr . David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]
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).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-02 21:47 ` Thomas Huth
2016-02-03 5:04 ` David Gibson
@ 2016-02-03 7:26 ` Markus Armbruster
1 sibling, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 7:26 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Stefan Hajnoczi, Lluís Vilanova,
Dr . David Alan Gilbert, David Gibson
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort
2016-02-02 18:53 ` Markus Armbruster
2016-02-02 21:47 ` Thomas Huth
@ 2016-02-03 17:59 ` Lluís Vilanova
1 sibling, 0 replies; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-03 17:59 UTC (permalink / raw)
To: Markus Armbruster
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v6 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
2016-02-02 16:13 [Qemu-devel] [RFC][PATCH v6 0/5] utils: Improve and document error reporting Lluís Vilanova
2016-02-02 16:13 ` [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort Lluís Vilanova
@ 2016-02-02 16:14 ` Lluís Vilanova
2016-02-02 20:16 ` John Snow
2016-02-02 16:14 ` [Qemu-devel] " Lluís Vilanova
` (2 subsequent siblings)
4 siblings, 1 reply; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:14 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Alexander Graf, Thomas Huth, open list:Floppy,
Stefan Hajnoczi, Dr . David Alan Gilbert, Markus Armbruster,
John Snow, open list:sPAPR, David Gibson
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 <vilanova@ac.upc.edu>
---
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(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e3b0e1e..8f0c947 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -347,9 +347,9 @@ static int pick_geometry(FDrive *drv)
/* No match of any kind found -- fd_format is misconfigured, abort. */
if (match == -1) {
- error_setg(&error_abort, "No candidate geometries present in table "
- " for floppy drive type '%s'",
- FloppyDriveType_lookup[drv->drive]);
+ error_report_abort("No candidate geometries present in table "
+ " for floppy drive type '%s'",
+ FloppyDriveType_lookup[drv->drive]);
}
parse = &(fd_formats[match]);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5bd8fd3..3c0f339 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -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 */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 90016e6..2228124 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -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);
diff --git a/util/error.c b/util/error.c
index 57303fd..b8a9120 100644
--- a/util/error.c
+++ b/util/error.c
@@ -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);
}
}
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort)
2016-02-02 16:14 ` [Qemu-devel] [PATCH v6 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort) Lluís Vilanova
@ 2016-02-02 20:16 ` John Snow
0 siblings, 0 replies; 32+ messages in thread
From: John Snow @ 2016-02-02 20:16 UTC (permalink / raw)
To: Lluís Vilanova, qemu-devel
Cc: Kevin Wolf, Thomas Huth, open list:Floppy, Stefan Hajnoczi,
Alexander Graf, Dr . David Alan Gilbert, open list:sPAPR,
Markus Armbruster, David Gibson
On 02/02/2016 11:14 AM, Lluís Vilanova wrote:
> 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 <vilanova@ac.upc.edu>
> ---
> 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(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index e3b0e1e..8f0c947 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -347,9 +347,9 @@ static int pick_geometry(FDrive *drv)
>
> /* No match of any kind found -- fd_format is misconfigured, abort. */
> if (match == -1) {
> - error_setg(&error_abort, "No candidate geometries present in table "
> - " for floppy drive type '%s'",
> - FloppyDriveType_lookup[drv->drive]);
> + error_report_abort("No candidate geometries present in table "
> + " for floppy drive type '%s'",
> + FloppyDriveType_lookup[drv->drive]);
> }
>
Acked-by: John Snow <jsnow@redhat.com>
> parse = &(fd_formats[match]);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5bd8fd3..3c0f339 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -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 */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 90016e6..2228124 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -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);
> diff --git a/util/error.c b/util/error.c
> index 57303fd..b8a9120 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -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);
> }
> }
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 3/5] util: [ppc] Use new error_report_fatal() instead of exit()
2016-02-02 16:13 [Qemu-devel] [RFC][PATCH v6 0/5] utils: Improve and document error reporting Lluís Vilanova
@ 2016-02-02 16:14 ` Lluís Vilanova
2016-02-02 16:14 ` [Qemu-devel] [PATCH v6 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort) Lluís Vilanova
` (3 subsequent siblings)
4 siblings, 0 replies; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:14 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Dr . David Alan Gilbert, Thomas Huth,
Markus Armbruster, Eric Blake, David Gibson, Alexander Graf,
Paolo Bonzini, open list:PowerPC, open list:Overall
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
target-ppc/kvm.c | 5 +--
target-ppc/translate.c | 7 ++--
target-ppc/translate_init.c | 80 +++++++++++++++++++++----------------------
3 files changed, 44 insertions(+), 48 deletions(-)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 70ca296..1bb87e6 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -316,9 +316,8 @@ static long gethugepagesize(const char *mem_path)
} while (ret != 0 && errno == EINTR);
if (ret != 0) {
- fprintf(stderr, "Couldn't statfs() memory path: %s\n",
- strerror(errno));
- exit(1);
+ error_report_fatal("Couldn't statfs() memory path: %s",
+ strerror(errno));
}
#define HUGETLBFS_MAGIC 0x958458f6
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 7db3145..546b21d 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11588,10 +11588,9 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
break;
}
if (tcg_check_temp_count()) {
- fprintf(stderr, "Opcode %02x %02x %02x (%08x) leaked temporaries\n",
- opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
- ctx.opcode);
- exit(1);
+ error_report_fatal("Opcode %02x %02x %02x (%08x) leaked temporaries",
+ opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
+ ctx.opcode);
}
}
if (tb->cflags & CF_LAST_IO)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index cdd18ac..d66be51 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -620,8 +620,8 @@ static inline void _spr_register(CPUPPCState *env, int num,
spr->oea_read != NULL || spr->oea_write != NULL ||
#endif
spr->uea_read != NULL || spr->uea_write != NULL) {
- printf("Error: Trying to register SPR %d (%03x) twice !\n", num, num);
- exit(1);
+ error_report_fatal("Error: Trying to register SPR %d (%03x) twice !",
+ num, num);
}
#if defined(PPC_DEBUG_SPR)
printf("*** register spr %d (%03x) %s val " TARGET_FMT_lx "\n", num, num,
@@ -1609,8 +1609,7 @@ static void gen_spr_BookE (CPUPPCState *env, uint64_t ivor_mask)
for (i = 0; i < 64; i++) {
if (ivor_mask & (1ULL << i)) {
if (ivor_sprn[i] == SPR_BOOKE_IVORxx) {
- fprintf(stderr, "ERROR: IVOR %d SPR is not defined\n", i);
- exit(1);
+ error_report_fatal("ERROR: IVOR %d SPR is not defined", i);
}
spr_register(env, ivor_sprn[i], ivor_names[i],
SPR_NOACCESS, SPR_NOACCESS,
@@ -8352,14 +8351,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_VRE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE");
}
} else if (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_SPE nor POWERPC_FLAG_VRE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_SPE nor POWERPC_FLAG_VRE");
}
if (env->msr_mask & (1 << 17)) {
switch (env->flags & (POWERPC_FLAG_TGPR | POWERPC_FLAG_CE)) {
@@ -8367,14 +8366,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_CE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_TGPR or POWERPC_FLAG_CE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_TGPR or POWERPC_FLAG_CE");
}
} else if (env->flags & (POWERPC_FLAG_TGPR | POWERPC_FLAG_CE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_TGPR nor POWERPC_FLAG_CE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_TGPR nor POWERPC_FLAG_CE");
}
if (env->msr_mask & (1 << 10)) {
switch (env->flags & (POWERPC_FLAG_SE | POWERPC_FLAG_DWE |
@@ -8384,17 +8383,17 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_UBLE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_SE or POWERPC_FLAG_DWE or "
- "POWERPC_FLAG_UBLE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_SE or POWERPC_FLAG_DWE or "
+ "POWERPC_FLAG_UBLE");
}
} else if (env->flags & (POWERPC_FLAG_SE | POWERPC_FLAG_DWE |
POWERPC_FLAG_UBLE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_SE nor POWERPC_FLAG_DWE nor "
- "POWERPC_FLAG_UBLE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_SE nor POWERPC_FLAG_DWE nor "
+ "POWERPC_FLAG_UBLE");
}
if (env->msr_mask & (1 << 9)) {
switch (env->flags & (POWERPC_FLAG_BE | POWERPC_FLAG_DE)) {
@@ -8402,14 +8401,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_DE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_BE or POWERPC_FLAG_DE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_BE or POWERPC_FLAG_DE");
}
} else if (env->flags & (POWERPC_FLAG_BE | POWERPC_FLAG_DE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_BE nor POWERPC_FLAG_DE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_BE nor POWERPC_FLAG_DE");
}
if (env->msr_mask & (1 << 2)) {
switch (env->flags & (POWERPC_FLAG_PX | POWERPC_FLAG_PMM)) {
@@ -8417,19 +8416,19 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_PMM:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_PX or POWERPC_FLAG_PMM\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_PX or POWERPC_FLAG_PMM");
}
} else if (env->flags & (POWERPC_FLAG_PX | POWERPC_FLAG_PMM)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_PX nor POWERPC_FLAG_PMM\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_PX nor POWERPC_FLAG_PMM");
}
if ((env->flags & (POWERPC_FLAG_RTC_CLK | POWERPC_FLAG_BUS_CLK)) == 0) {
- fprintf(stderr, "PowerPC flags inconsistency\n"
- "Should define the time-base and decrementer clock source\n");
- exit(1);
+ error_report("PowerPC flags inconsistency");
+ error_report_fatal(
+ "Should define the time-base and decrementer clock source");
}
/* Allocate TLBs buffer when needed */
#if !defined(CONFIG_USER_ONLY)
@@ -9655,8 +9654,7 @@ static void ppc_cpu_reset(CPUState *s)
#if !defined(TARGET_WORDS_BIGENDIAN)
msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
if (!((env->msr_mask >> MSR_LE) & 1)) {
- fprintf(stderr, "Selected CPU does not support little-endian.\n");
- exit(1);
+ error_report_fatal("Selected CPU does not support little-endian.");
}
#endif
#endif
^ permalink raw reply related [flat|nested] 32+ messages in thread* [Qemu-devel] [PATCH v6 3/5] util: [ppc] Use new error_report_fatal() instead of exit()
@ 2016-02-02 16:14 ` Lluís Vilanova
0 siblings, 0 replies; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Graf, Thomas Huth, open list:Overall, Stefan Hajnoczi,
Dr . David Alan Gilbert, Markus Armbruster, open list:PowerPC,
Paolo Bonzini, David Gibson
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
target-ppc/kvm.c | 5 +--
target-ppc/translate.c | 7 ++--
target-ppc/translate_init.c | 80 +++++++++++++++++++++----------------------
3 files changed, 44 insertions(+), 48 deletions(-)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 70ca296..1bb87e6 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -316,9 +316,8 @@ static long gethugepagesize(const char *mem_path)
} while (ret != 0 && errno == EINTR);
if (ret != 0) {
- fprintf(stderr, "Couldn't statfs() memory path: %s\n",
- strerror(errno));
- exit(1);
+ error_report_fatal("Couldn't statfs() memory path: %s",
+ strerror(errno));
}
#define HUGETLBFS_MAGIC 0x958458f6
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 7db3145..546b21d 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11588,10 +11588,9 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
break;
}
if (tcg_check_temp_count()) {
- fprintf(stderr, "Opcode %02x %02x %02x (%08x) leaked temporaries\n",
- opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
- ctx.opcode);
- exit(1);
+ error_report_fatal("Opcode %02x %02x %02x (%08x) leaked temporaries",
+ opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
+ ctx.opcode);
}
}
if (tb->cflags & CF_LAST_IO)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index cdd18ac..d66be51 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -620,8 +620,8 @@ static inline void _spr_register(CPUPPCState *env, int num,
spr->oea_read != NULL || spr->oea_write != NULL ||
#endif
spr->uea_read != NULL || spr->uea_write != NULL) {
- printf("Error: Trying to register SPR %d (%03x) twice !\n", num, num);
- exit(1);
+ error_report_fatal("Error: Trying to register SPR %d (%03x) twice !",
+ num, num);
}
#if defined(PPC_DEBUG_SPR)
printf("*** register spr %d (%03x) %s val " TARGET_FMT_lx "\n", num, num,
@@ -1609,8 +1609,7 @@ static void gen_spr_BookE (CPUPPCState *env, uint64_t ivor_mask)
for (i = 0; i < 64; i++) {
if (ivor_mask & (1ULL << i)) {
if (ivor_sprn[i] == SPR_BOOKE_IVORxx) {
- fprintf(stderr, "ERROR: IVOR %d SPR is not defined\n", i);
- exit(1);
+ error_report_fatal("ERROR: IVOR %d SPR is not defined", i);
}
spr_register(env, ivor_sprn[i], ivor_names[i],
SPR_NOACCESS, SPR_NOACCESS,
@@ -8352,14 +8351,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_VRE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE");
}
} else if (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_SPE nor POWERPC_FLAG_VRE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_SPE nor POWERPC_FLAG_VRE");
}
if (env->msr_mask & (1 << 17)) {
switch (env->flags & (POWERPC_FLAG_TGPR | POWERPC_FLAG_CE)) {
@@ -8367,14 +8366,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_CE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_TGPR or POWERPC_FLAG_CE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_TGPR or POWERPC_FLAG_CE");
}
} else if (env->flags & (POWERPC_FLAG_TGPR | POWERPC_FLAG_CE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_TGPR nor POWERPC_FLAG_CE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_TGPR nor POWERPC_FLAG_CE");
}
if (env->msr_mask & (1 << 10)) {
switch (env->flags & (POWERPC_FLAG_SE | POWERPC_FLAG_DWE |
@@ -8384,17 +8383,17 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_UBLE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_SE or POWERPC_FLAG_DWE or "
- "POWERPC_FLAG_UBLE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_SE or POWERPC_FLAG_DWE or "
+ "POWERPC_FLAG_UBLE");
}
} else if (env->flags & (POWERPC_FLAG_SE | POWERPC_FLAG_DWE |
POWERPC_FLAG_UBLE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_SE nor POWERPC_FLAG_DWE nor "
- "POWERPC_FLAG_UBLE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_SE nor POWERPC_FLAG_DWE nor "
+ "POWERPC_FLAG_UBLE");
}
if (env->msr_mask & (1 << 9)) {
switch (env->flags & (POWERPC_FLAG_BE | POWERPC_FLAG_DE)) {
@@ -8402,14 +8401,14 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_DE:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_BE or POWERPC_FLAG_DE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_BE or POWERPC_FLAG_DE");
}
} else if (env->flags & (POWERPC_FLAG_BE | POWERPC_FLAG_DE)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_BE nor POWERPC_FLAG_DE\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_BE nor POWERPC_FLAG_DE");
}
if (env->msr_mask & (1 << 2)) {
switch (env->flags & (POWERPC_FLAG_PX | POWERPC_FLAG_PMM)) {
@@ -8417,19 +8416,19 @@ static void init_ppc_proc(PowerPCCPU *cpu)
case POWERPC_FLAG_PMM:
break;
default:
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should define POWERPC_FLAG_PX or POWERPC_FLAG_PMM\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should define POWERPC_FLAG_PX or POWERPC_FLAG_PMM");
}
} else if (env->flags & (POWERPC_FLAG_PX | POWERPC_FLAG_PMM)) {
- fprintf(stderr, "PowerPC MSR definition inconsistency\n"
- "Should not define POWERPC_FLAG_PX nor POWERPC_FLAG_PMM\n");
- exit(1);
+ error_report("PowerPC MSR definition inconsistency");
+ error_report_fatal(
+ "Should not define POWERPC_FLAG_PX nor POWERPC_FLAG_PMM");
}
if ((env->flags & (POWERPC_FLAG_RTC_CLK | POWERPC_FLAG_BUS_CLK)) == 0) {
- fprintf(stderr, "PowerPC flags inconsistency\n"
- "Should define the time-base and decrementer clock source\n");
- exit(1);
+ error_report("PowerPC flags inconsistency");
+ error_report_fatal(
+ "Should define the time-base and decrementer clock source");
}
/* Allocate TLBs buffer when needed */
#if !defined(CONFIG_USER_ONLY)
@@ -9655,8 +9654,7 @@ static void ppc_cpu_reset(CPUState *s)
#if !defined(TARGET_WORDS_BIGENDIAN)
msr |= (target_ulong)1 << MSR_LE; /* Little-endian user mode */
if (!((env->msr_mask >> MSR_LE) & 1)) {
- fprintf(stderr, "Selected CPU does not support little-endian.\n");
- exit(1);
+ error_report_fatal("Selected CPU does not support little-endian.");
}
#endif
#endif
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v6 4/5] util: [ppc] Use new error_report_abort() instead of abort()
2016-02-02 16:13 [Qemu-devel] [RFC][PATCH v6 0/5] utils: Improve and document error reporting Lluís Vilanova
@ 2016-02-02 16:14 ` Lluís Vilanova
2016-02-02 16:14 ` [Qemu-devel] [PATCH v6 2/5] util: Use new error_report_fatal/abort instead of error_setg(&error_fatal/abort) Lluís Vilanova
` (3 subsequent siblings)
4 siblings, 0 replies; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:14 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Dr . David Alan Gilbert, Thomas Huth,
Markus Armbruster, Eric Blake, David Gibson, Alexander Graf,
Paolo Bonzini, open list:PowerPC, open list:Overall
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
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(-)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 1bb87e6..b743811 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -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, ®);
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 62406ce..f82582e 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -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
@@ -215,36 +218,36 @@ static inline int kvmppc_get_htab_fd(bool write)
static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
int64_t max_ns)
{
- abort();
+ error_report_abort(" ");
}
static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
uint16_t n_valid, uint16_t n_invalid)
{
- abort();
+ error_report_abort(" ");
}
static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
target_ulong pte_index)
{
- abort();
+ error_report_abort(" ");
}
static inline void kvmppc_hash64_free_pteg(uint64_t token)
{
- abort();
+ error_report_abort(" ");
}
static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
target_ulong pte_index,
target_ulong pte0, target_ulong pte1)
{
- abort();
+ error_report_abort(" ");
}
static inline bool kvmppc_has_cap_fixup_hcalls(void)
{
- abort();
+ error_report_abort(" ");
}
static inline int kvmppc_enable_hwrng(void)
diff --git a/target-ppc/mmu-hash32.c b/target-ppc/mmu-hash32.c
index b076d80..75e593a 100644
--- a/target-ppc/mmu-hash32.c
+++ b/target-ppc/mmu-hash32.c
@@ -21,6 +21,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
#include "exec/helper-proto.h"
+#include "qemu/error-report.h"
#include "sysemu/kvm.h"
#include "kvm_ppc.h"
#include "mmu-hash32.h"
@@ -56,7 +57,7 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)
break;
default:
- abort();
+ error_report_abort("Unhandled pp: %d", pp);
}
} else {
switch (pp) {
@@ -74,7 +75,7 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)
break;
default:
- abort();
+ error_report_abort("Unhandled pp: %d", pp);
}
}
if (nx == 0) {
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index de4e286..6e50e78 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1324,8 +1324,7 @@ static inline int check_physical(CPUPPCState *env, mmu_ctx_t *ctx,
default:
/* Caller's checks mean we should never get here for other models */
- abort();
- return -1;
+ error_report_abort("Unhandled MMU model: %d", env->mmu_model);
}
return ret;
^ permalink raw reply related [flat|nested] 32+ messages in thread* [Qemu-devel] [PATCH v6 4/5] util: [ppc] Use new error_report_abort() instead of abort()
@ 2016-02-02 16:14 ` Lluís Vilanova
0 siblings, 0 replies; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:14 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Graf, Thomas Huth, open list:Overall, Stefan Hajnoczi,
Dr . David Alan Gilbert, Markus Armbruster, open list:PowerPC,
Paolo Bonzini, David Gibson
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
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(-)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 1bb87e6..b743811 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -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, ®);
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 62406ce..f82582e 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -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
@@ -215,36 +218,36 @@ static inline int kvmppc_get_htab_fd(bool write)
static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
int64_t max_ns)
{
- abort();
+ error_report_abort(" ");
}
static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
uint16_t n_valid, uint16_t n_invalid)
{
- abort();
+ error_report_abort(" ");
}
static inline uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu,
target_ulong pte_index)
{
- abort();
+ error_report_abort(" ");
}
static inline void kvmppc_hash64_free_pteg(uint64_t token)
{
- abort();
+ error_report_abort(" ");
}
static inline void kvmppc_hash64_write_pte(CPUPPCState *env,
target_ulong pte_index,
target_ulong pte0, target_ulong pte1)
{
- abort();
+ error_report_abort(" ");
}
static inline bool kvmppc_has_cap_fixup_hcalls(void)
{
- abort();
+ error_report_abort(" ");
}
static inline int kvmppc_enable_hwrng(void)
diff --git a/target-ppc/mmu-hash32.c b/target-ppc/mmu-hash32.c
index b076d80..75e593a 100644
--- a/target-ppc/mmu-hash32.c
+++ b/target-ppc/mmu-hash32.c
@@ -21,6 +21,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
#include "exec/helper-proto.h"
+#include "qemu/error-report.h"
#include "sysemu/kvm.h"
#include "kvm_ppc.h"
#include "mmu-hash32.h"
@@ -56,7 +57,7 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)
break;
default:
- abort();
+ error_report_abort("Unhandled pp: %d", pp);
}
} else {
switch (pp) {
@@ -74,7 +75,7 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)
break;
default:
- abort();
+ error_report_abort("Unhandled pp: %d", pp);
}
}
if (nx == 0) {
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index de4e286..6e50e78 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1324,8 +1324,7 @@ static inline int check_physical(CPUPPCState *env, mmu_ctx_t *ctx,
default:
/* Caller's checks mean we should never get here for other models */
- abort();
- return -1;
+ error_report_abort("Unhandled MMU model: %d", env->mmu_model);
}
return ret;
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v6 4/5] util: [ppc] Use new error_report_abort() instead of abort()
2016-02-02 16:14 ` [Qemu-devel] " Lluís Vilanova
@ 2016-02-02 19:34 ` Eric Blake
-1 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2016-02-02 19:34 UTC (permalink / raw)
To: Lluís Vilanova, qemu-devel
Cc: Stefan Hajnoczi, Dr . David Alan Gilbert, Thomas Huth,
Markus Armbruster, David Gibson, Alexander Graf, Paolo Bonzini,
open list:PowerPC, open list:Overall
[-- Attachment #1: Type: text/plain, Size: 984 bytes --]
On 02/02/2016 09:14 AM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 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(-)
>
> @@ -215,36 +218,36 @@ static inline int kvmppc_get_htab_fd(bool write)
> static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
> int64_t max_ns)
> {
> - abort();
> + error_report_abort(" ");
> }
I still strongly dislike this. Reporting an error message with nothing
but a trailing whitespace is NOT helpful. Straight 'abort()' is better
than this; if we can't come up with a useful message, then it is not
worth using the message-handling interface.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 4/5] util: [ppc] Use new error_report_abort() instead of abort()
@ 2016-02-02 19:34 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2016-02-02 19:34 UTC (permalink / raw)
To: Lluís Vilanova, qemu-devel
Cc: Thomas Huth, open list:Overall, Stefan Hajnoczi,
Dr . David Alan Gilbert, Alexander Graf, open list:PowerPC,
Paolo Bonzini, Markus Armbruster, David Gibson
[-- Attachment #1: Type: text/plain, Size: 984 bytes --]
On 02/02/2016 09:14 AM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 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(-)
>
> @@ -215,36 +218,36 @@ static inline int kvmppc_get_htab_fd(bool write)
> static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
> int64_t max_ns)
> {
> - abort();
> + error_report_abort(" ");
> }
I still strongly dislike this. Reporting an error message with nothing
but a trailing whitespace is NOT helpful. Straight 'abort()' is better
than this; if we can't come up with a useful message, then it is not
worth using the message-handling interface.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 4/5] util: [ppc] Use new error_report_abort() instead of abort()
2016-02-02 16:14 ` [Qemu-devel] " Lluís Vilanova
@ 2016-02-03 5:06 ` David Gibson
-1 siblings, 0 replies; 32+ messages in thread
From: David Gibson @ 2016-02-03 5:06 UTC (permalink / raw)
To: Lluís Vilanova
Cc: qemu-devel, Stefan Hajnoczi, Dr . David Alan Gilbert,
Thomas Huth, Markus Armbruster, Eric Blake, Alexander Graf,
Paolo Bonzini, open list:PowerPC, open list:Overall
[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]
On Tue, Feb 02, 2016 at 05:14:15PM +0100, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 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(-)
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 1bb87e6..b743811 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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, ®);
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 62406ce..f82582e 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -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
> @@ -215,36 +218,36 @@ static inline int kvmppc_get_htab_fd(bool write)
> static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
> int64_t max_ns)
> {
> - abort();
> + error_report_abort(" ");
Nack. The empty message report is just awful. I don't think we
should feel the need to get rid of every bare abort() or assert().
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 4/5] util: [ppc] Use new error_report_abort() instead of abort()
@ 2016-02-03 5:06 ` David Gibson
0 siblings, 0 replies; 32+ messages in thread
From: David Gibson @ 2016-02-03 5:06 UTC (permalink / raw)
To: Lluís Vilanova
Cc: Alexander Graf, Thomas Huth, open list:Overall, Stefan Hajnoczi,
qemu-devel, Markus Armbruster, open list:PowerPC, Paolo Bonzini,
Dr . David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]
On Tue, Feb 02, 2016 at 05:14:15PM +0100, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 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(-)
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 1bb87e6..b743811 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -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, ®);
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 62406ce..f82582e 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -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
> @@ -215,36 +218,36 @@ static inline int kvmppc_get_htab_fd(bool write)
> static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
> int64_t max_ns)
> {
> - abort();
> + error_report_abort(" ");
Nack. The empty message report is just awful. I don't think we
should feel the need to get rid of every bare abort() or assert().
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-02 16:13 [Qemu-devel] [RFC][PATCH v6 0/5] utils: Improve and document error reporting Lluís Vilanova
` (3 preceding siblings ...)
2016-02-02 16:14 ` [Qemu-devel] " Lluís Vilanova
@ 2016-02-02 16:14 ` Lluís Vilanova
2016-02-02 19:10 ` Markus Armbruster
2016-02-03 16:58 ` Markus Armbruster
4 siblings, 2 replies; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-02 16:14 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Stefan Hajnoczi, Dr . David Alan Gilbert,
Markus Armbruster, David Gibson
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
HACKING | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/HACKING b/HACKING
index 12fbc8a..b738bce 100644
--- a/HACKING
+++ b/HACKING
@@ -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()').
+
+As a general rule, use the 'fatal' forms below for errors that can be triggered
+by the user (e.g., commandline, hotplug, etc.), and the 'abort' forms for
+programming errors that the user should not be able to trigger.
+
+7.1. Simple error messages
+
+The 'error_report*()' functions in "include/qemu/error-report.h" will
+immediately report error messages to the user.
+
+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()'.
+
+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).
+
+7.3. More complex error management
+
+The functions in "include/qapi/error.h" can be used to accumulate error messages
+in an 'Error' object, which can be propagated up the call chain where it is
+finally reported.
+
+WARNING: The special 'error_fatal' and 'error_abort' objects follow the same
+constraints as the 'error_report_fatal()' and 'error_report_abort()' functions.
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-02 16:14 ` [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors Lluís Vilanova
@ 2016-02-02 19:10 ` Markus Armbruster
2016-02-03 13:47 ` Lluís Vilanova
2016-02-03 16:58 ` Markus Armbruster
1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2016-02-02 19:10 UTC (permalink / raw)
To: Lluís Vilanova
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Gives some general guidelines for reporting errors in QEMU.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> HACKING | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/HACKING b/HACKING
> index 12fbc8a..b738bce 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -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.
> +
> +As a general rule, use the 'fatal' forms below for errors that can be triggered
Where's "below"?
> +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.
> +
> +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".
> +
> +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.
> +
> +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.
> +
> +7.3. More complex error management
> +
> +The functions in "include/qapi/error.h" can be used to accumulate error messages
Long line.
> +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".
> +
> +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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-02 19:10 ` Markus Armbruster
@ 2016-02-03 13:47 ` Lluís Vilanova
2016-02-03 14:41 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-03 13:47 UTC (permalink / raw)
To: Markus Armbruster
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
Markus Armbruster writes:
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Gives some general guidelines for reporting errors in QEMU.
>>
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> HACKING | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/HACKING b/HACKING
>> index 12fbc8a..b738bce 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -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.
>> +
>> +As a general rule, use the 'fatal' forms below for errors that can be triggered
> Where's "below"?
It's the following two sections, sorry.
>> +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.
>> +
>> +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".
>> +
>> +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.
>> +
>> +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.
I can try adding that, although I've barely used this part of the interface.
>> +
>> +7.3. More complex error management
>> +
>> +The functions in "include/qapi/error.h" can be used to accumulate error messages
> Long line.
>> +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".
>> +
>> +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.
Thanks,
Lluis
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-03 13:47 ` Lluís Vilanova
@ 2016-02-03 14:41 ` Markus Armbruster
2016-02-03 15:17 ` Lluís Vilanova
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 14:41 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Thomas Huth, Dr . David Alan Gilbert, David Gibson
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Gives some general guidelines for reporting errors in QEMU.
>>>
>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>> ---
>>> HACKING | 37 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 37 insertions(+)
>>>
>>> diff --git a/HACKING b/HACKING
>>> index 12fbc8a..b738bce 100644
>>> --- a/HACKING
>>> +++ b/HACKING
[...]
>>> +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.
>
> 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.
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-03 14:41 ` Markus Armbruster
@ 2016-02-03 15:17 ` Lluís Vilanova
2016-02-03 15:53 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Lluís Vilanova @ 2016-02-03 15:17 UTC (permalink / raw)
To: Markus Armbruster
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
Markus Armbruster writes:
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>>
>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>> Gives some general guidelines for reporting errors in QEMU.
>>>>
>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> ---
>>>> HACKING | 37 +++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/HACKING b/HACKING
>>>> index 12fbc8a..b738bce 100644
>>>> --- a/HACKING
>>>> +++ b/HACKING
> [...]
>>>> +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.
>>
>> 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 :)
Thanks,
Lluis
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-03 15:17 ` Lluís Vilanova
@ 2016-02-03 15:53 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 15:53 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Thomas Huth, Dr . David Alan Gilbert, David Gibson
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:
>>>>> Gives some general guidelines for reporting errors in QEMU.
>>>>>
>>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>>> ---
>>>>> HACKING | 37 +++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 37 insertions(+)
>>>>>
>>>>> diff --git a/HACKING b/HACKING
>>>>> index 12fbc8a..b738bce 100644
>>>>> --- a/HACKING
>>>>> +++ b/HACKING
>> [...]
>>>>> +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.
>>>
>>> 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 :)
If you want to cover error locations in HACKING briefly, that's okay
with me, but it needs to be correct. I'll look over your proposed patch
again.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors
2016-02-02 16:14 ` [Qemu-devel] [PATCH v6 5/5] doc: Introduce coding style for errors Lluís Vilanova
2016-02-02 19:10 ` Markus Armbruster
@ 2016-02-03 16:58 ` Markus Armbruster
1 sibling, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2016-02-03 16:58 UTC (permalink / raw)
To: Lluís Vilanova
Cc: Stefan Hajnoczi, Thomas Huth, David Gibson, qemu-devel,
Dr . David Alan Gilbert
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Gives some general guidelines for reporting errors in QEMU.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
I started re-reviewing this, but my suggestions quickly became a rewrite
(sorry), so I'm just posting just that. I'll turn it into a proper
patch you can include in your series.
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().
^ permalink raw reply [flat|nested] 32+ messages in thread