Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
> 16.04.2019 9:34, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
>>
>>> 15.04.2019 16:08, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
>>>>
>>>>> 15.04.2019 8:51, Markus Armbruster wrote:
>>>>>> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
>>>>>>
>>>>>>> It would be nice to have Error object not freed away when debugging a
>>>>>>> coredump.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>>>>>> ---
>>>>>>> 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);
>>
>
> Hmm, this means, that in error_report_err checking for qmp monitor is wrong
> thing,
> as error_report_err is exactly for human user who will read qemu log and will
> need
> maximum information.
You're right. I'll post a patch.
