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

[Vladimir Sementsov-Ogievskiy]

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?

> 
> Intentional?  It makes sense as an error message, but readability
> suffers due to the long line.  If we decide we want this change, it
> needs to be mentioned in the commit message.
> 
> Open-coding error_report_err() here so you can delete the error_free()
> and a newline is a bit ugly, but factoring out the common part doesn't
> seem worthwhile.
> 
> Hmm, perhaps factoring out
> 
>          if (err->hint) {
>              error_printf_unless_qmp("%s", err->hint->str);
>          }
> 
> into a static helper would be worthwhile.  We already have two copies.
> 


-- 
Best regards,
Vladimir