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

[Vladimir Sementsov-Ogievskiy]

16.04.2019 9:34, Markus Armbruster wrote:
> 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);
> 

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.

-- 
Best regards,
Vladimir