Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: > 15.04.2019 16:08, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: >> >>> 15.04.2019 8:51, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: >>>> >>>>> It would be nice to have Error object not freed away when debugging a >>>>> coredump. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@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);