util/error.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
It would be nice to have Error object not freed away when debugging a
coredump.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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) {
--
2.18.0
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> It would be nice to have Error object not freed away when debugging a
> coredump.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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)
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.
15.04.2019 8:51, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> It would be nice to have Error object not freed away when debugging a
>> coredump.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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?
>
> 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
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 15.04.2019 8:51, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> It would be nice to have Error object not freed away when debugging a
>>> coredump.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
15.04.2019 16:08, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 15.04.2019 8:51, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> It would be nice to have Error object not freed away when debugging a
>>>> coredump.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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"?
--
Best regards,
Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 15.04.2019 16:08, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 15.04.2019 8:51, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> It would be nice to have Error object not freed away when debugging a
>>>>> coredump.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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);
16.04.2019 9:34, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 15.04.2019 16:08, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 15.04.2019 8:51, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>>
>>>>>> It would be nice to have Error object not freed away when debugging a
>>>>>> coredump.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
--
Best regards,
Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 16.04.2019 9:34, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 15.04.2019 16:08, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> 15.04.2019 8:51, Markus Armbruster wrote:
>>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>>>
>>>>>>> It would be nice to have Error object not freed away when debugging a
>>>>>>> coredump.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
© 2016 - 2024 Red Hat, Inc.