[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] util/error: do not free error on error_abort
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] util/error: do not free error on error_abort |
Date: |
Tue, 16 Apr 2019 08:34:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 15.04.2019 16:08, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
>>
>>> 15.04.2019 8:51, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
>>>>
>>>>> It would be nice to have Error object not freed away when debugging a
>>>>> coredump.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>> ---
>>>>> util/error.c | 8 +++++---
>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/util/error.c b/util/error.c
>>>>> index 934a78e1b1..f9180c0f30 100644
>>>>> --- a/util/error.c
>>>>> +++ b/util/error.c
>>>>> @@ -32,9 +32,11 @@ Error *error_fatal;
>>>>> static void error_handle_fatal(Error **errp, Error *err)
>>>>> {
>>>>> if (errp == &error_abort) {
>>>>> - fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>>>>> - err->func, err->src, err->line);
>>>>> - error_report_err(err);
>>>>> + error_report("Unexpected error in %s() at %s:%d: %s",
>>>>> + err->func, err->src, err->line,
>>>>> error_get_pretty(err));
>>>>> + if (err->hint) {
>>>>> + error_printf_unless_qmp("%s", err->hint->str);
>>>>> + }
>>>>> abort();
>>>>> }
>>>>> if (errp == &error_fatal) {
>>>>
>>>> No objections to not freeing the error object on the path to abort().
>>>>
>>>> You also format the error message differently. Commit 1e9b65bb1ba's
>>>> example
>>>>
>>>> Unexpected error in parse_block_error_action() at
>>>> .../qemu/blockdev.c:322:
>>>> qemu-system-x86_64: -drive if=none,werror=foo: 'foo' invalid
>>>> write error action
>>>> Aborted (core dumped)
>>>>
>>>> changes to (guesswork, not tested):
>>>>
>>>> qemu-system-x86_64: -drive if=none,werror=foo: Unexpected error
>>>> in parse_block_error_action() at .../qemu/blockdev.c:322: 'foo' invalid
>>>> write error action
>>>> Aborted (core dumped)
>>>
>>> hm. checkpatch don't allow to put \n into error_report. So, should I call
>>> error_report twice?
>>>
>>> Transition from fprintf to error_report is OK for you?
>>
>> Let's simply not mess with the formatting at all. Something like:
>>
>> (1) Inline error_report_err(), delete the error_free()
>>
>> (2) Optional: replace fprintf() by error_printf()
>>
>> (3) Optional: clean up the additional copy of
>>
>> if (err->hint) {
>> error_printf_unless_qmp("%s", err->hint->str);
>> }
>>
>> (3a) Either create a helper function, replacing all three copies,
>>
>> (3b) Or simply delete the new copy from the path leading to abort(),
>> since the hint is unlikely to be useful there.
>>
>
> Why we print error always but hint only "unless_qmp"?
"Hints" are intended for giving hints to a human user (although they are
misused for other purposes in places):
/*
* Append a printf-style human-readable explanation to an existing error.
* If the error is later reported to a human user with
* error_report_err() or warn_report_err(), the hints will be shown,
* too. If it's reported via QMP, the hints will be ignored.
* Intended use is adding helpful hints on the human user interface,
* e.g. a list of valid values. It's not for clarifying a confusing
* error message.
* @errp may be NULL, but not &error_fatal or &error_abort.
* Trivially the case if you call it only after error_setg() or
* error_propagate().
* May be called multiple times. The resulting hint should end with a
* newline.
*/
void error_append_hint(Error **errp, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);