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

[Markus Armbruster]

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.