Re: [Qemu-devel] [PATCH] util/error: do not free error on error_abort

[Vladimir Sementsov-Ogievskiy]

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"?


-- 
Best regards,
Vladimir