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

[Markus Armbruster]

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)

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.