Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort

[Markus Armbruster <armbru@redhat.com>]

I'm struggling with my review queue, and have had to resort to subsystem
batching to increase throughput.  Because of that, v3-v5 have flown by
without a peep from me.  My sincere apologies.

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Provide two lean functions to report error messages that fatal/abort
> QEMU.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  include/qemu/error-report.h |   19 +++++++++++++++++++
>  util/qemu-error.c           |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 7ab2355..6c2f142 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  const char *error_get_progname(void);
>  extern bool enable_timestamp_msg;
>  
> +/* Report message and exit with error */
> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) GCC_FMT_ATTR(1, 2);

This lets people write things like

    error_report_fatal("The sky is falling");

instead of

    error_report("The sky is falling");
    exit(1);

or

    fprintf(stderr, "The sky is falling\n");
    exit(1);

I don't think that's an improvement in clarity.

PATCH 3 actually does this for a couple of cases.

> +/* Report message with caller location and abort */
> +#define error_vreport_abort(fmt, ap)                                    \
> +    do {                                                                \
> +        error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \
> +        error_vreport_abort_internal(fmt, ap);                          \
> +    } while (0)
> +#define error_report_abort(fmt, ...)                                    \
> +    do {                                                                \
> +        error_report_abort_caller_internal(__FILE__, __LINE__, __func__); \
> +        error_report_abort_internal(fmt, ##__VA_ARGS__);                \
> +    } while (0)
> +
> +void error_report_abort_caller_internal(const char *file, int line, const char *func);
> +void QEMU_NORETURN error_vreport_abort_internal(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
> +void QEMU_NORETURN error_report_abort_internal(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> +
>  #endif

Feature not mentioned in the commit message: your new form prints an
additional line with source location information, like &error_abort
does.  See error_report_abort_internal() below.

PATCH 4 uses error_report_abort() to change a dozen

    abort()

to

    error_report_abort(... some message ...);

or even

    error_report_abort(" ");

Two aspects: source code and behavior.

Source code: I don't think it makes it more readable.

Behavior: prints a message before it crashes.

The messages look like they make sense only to developers, not to users.
This isn't surprising; these are internal errors, and you generally
can't explain internal errors without referring to internal concepts the
user doesn't know about.  Should they happen, you need to debug anyway.

As to the " " messages: surely you're joking, Mr. Feynman :)

I feel the message adds very little information to the core dump for
developers, and none for users.

If the error message is genuinely useful to users, chances are we should
exit(1) rather than abort().

If the QEMU developer community should decide I'm wrong and we really
want to decorate abort()s with messages, we'd have to decorate the
majority of the 600+ we have so that people can see the pattern.
Without that, new undecorated ones will pop up faster than we can
decorate them.  In other words, the dozen you converted to demonstrate
the idea for your RFC are a placeholder for one of these tree-wide
transitions that are easier to start than to finish.

PATCH 2 uses error_report_abort() to clean up

    error_setg(&error_abort, ... some message ...);

to

    error_report_abort(... some message ...);

I agree these uses of &error_abort should be cleaned up.  However, I'd
clean them up to abort() or assert(), for the reasons I just explained,
and because that's what we do elsewhere.

> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index ecf5708..3de002b 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -237,3 +237,36 @@ void error_report(const char *fmt, ...)
>      error_vreport(fmt, ap);
>      va_end(ap);
>  }
> +
> +void error_vreport_fatal(const char *fmt, va_list ap)
> +{
> +    error_vreport(fmt, ap);
> +    exit(1);
> +}
> +
> +void error_report_fatal(const char *fmt, ...)
> +{
> +    va_list ap;
> +    va_start(ap, fmt);
> +    error_vreport_fatal(fmt, ap);
> +    va_end(ap);
> +}
> +
> +void error_report_abort_caller_internal(const char *file, int line, const char *func)
> +{
> +    error_report("Unexpected error in %s() at %s:%d:", func, file, line);

Should use error_printf(), so we get

    Unexpected error in frobnicate() at frob.c:666:
    qemu-system-x86_64: --frobnicate: Out of frobs

instead of

    qemu-system-x86_64: --frobnicate: Unexpected error in frobnicate() at frob.c:666:
    qemu-system-x86_64: --frobnicate: Out of frobs

> +}
> +
> +void error_vreport_abort_internal(const char *fmt, va_list ap)
> +{
> +    error_vreport(fmt, ap);
> +    abort();
> +}
> +
> +void error_report_abort_internal(const char *fmt, ...)
> +{
> +    va_list ap;
> +    va_start(ap, fmt);
> +    error_vreport_abort_internal(fmt, ap);
> +    va_end(ap);
> +}