Re: [Qemu-devel] [PATCH v2 1/2] utils: Add warning messages

[Markus Armbruster]

Lluís Vilanova <address@hidden> writes:

> Adds a special error object that transforms error messages into
> immediately reported warnings.

Before I dive into details: my fundamental objection is that &error_warn
is new infrastructure without a use.  The new "this is how you should do
warnings" paragraph in error.h's big comment does not count as use :)

Without actual use, we can't be sure it's useful.  And being useful
should be mandatory for new infrastructure, even when it's as small as
this one.

Finally, note that

    err = NULL;
    foo(arg, &err);
    if (err) {
        error_report("warning: %s", error_get_pretty(err));
        error_free(err);
    }

and

    foo(arg, &error_warn)

are subtly different.

One, the former throws away the hint.  That may or may not be
appropriate.  It also happens in other places that amend an error's
message.  Perhaps we could use a function to amend an error's message.
To find out, we'd have to track down the places that do that now.

Two, the latter doesn't let the caller see whether foo() warned.  I
believe this makes it unsuitable for some cases, or in other words, it's
insufficiently general.

Three, the latter can't catch misuse the former catches, namely multiple
error_set().  The latter happily reports all of them, the former fails
error_setv()'s assertion for the second one.

If warnings are common enough to justify infrastructure, then I figure a
convenience function to report an Error as a warning similar to
error_report_err() would be more general and less prone to misuse.

> Signed-off-by: Lluís Vilanova <address@hidden>
> ---
>  include/qapi/error.h |   20 ++++++++++++++++++++
>  util/error.c         |   37 +++++++++++++++++++++++++++----------
>  2 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 4d42cdc..9b7600c 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -57,6 +57,9 @@
>   * Call a function treating errors as fatal:
>   *     foo(arg, &error_fatal);
>   *
> + * Call a function immediately showing all errors as warnings:
> + *     foo(arg, &error_warn);
> + *
>   * Receive an error and pass it on to the caller:
>   *     Error *err = NULL;
>   *     foo(arg, &err);
> @@ -108,6 +111,7 @@ ErrorClass error_get_class(const Error *err);
>   * then.
>   * If @errp is &error_abort, print a suitable message and abort().
>   * If @errp is &error_fatal, print a suitable message and exit(1).
> + * If @errp is &error_warn, print a suitable message.
>   * If @errp is anything else, address@hidden must be NULL.
>   * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
>   * human-readable error message is made from printf-style @fmt, ...
> @@ -158,6 +162,7 @@ void error_setg_win32_internal(Error **errp,
>   * abort().
>   * Else, if @dst_errp is &error_fatal, print a suitable message and
>   * exit(1).
> + * Else, if @dst_errp is &error_warn, print a suitable message.
>   * Else, if @dst_errp already contains an error, ignore this one: free
>   * the error object.
>   * Else, move the error object from @local_err to address@hidden
> @@ -218,12 +223,27 @@ void error_set_internal(Error **errp,
>  
>  /*
>   * Pass to error_setg() & friends to abort() on error.
> + *
> + * WARNING: Do _not_ use for errors that are (or can be) triggered by guest 
> code
> + *          (e.g., some unimplimented corner case in guest code translation 
> or
> + *          device code). Otherwise that can be abused by guest code to
> + *          terminate QEMU.
>   */
>  extern Error *error_abort;
>  
>  /*
>   * Pass to error_setg() & friends to exit(1) on error.
> + *
> + * WARNING: Do _not_ use for errors that are (or can be) triggered by guest 
> code
> + *          (e.g., some unimplimented corner case in guest code translation 
> or
> + *          device code). Otherwise that can be abused by guest code to
> + *          terminate QEMU.
>   */
>  extern Error *error_fatal;

This hunk isn't covered by the commit message.

Similar admonitions elsewhere in this file are less ornately decorated.
Plain

 * Do *not* use for errors that are (or can be) triggered by guest code
 * (e.g., some unimplemented corner case in guest code translation or
 * device code). Otherwise that can be abused by guest code to terminate
 * QEMU.

would do, and avoid the long lines.

Except this wording is too narrow.  There are many more places where
"terminate on error" is wrong, such as monitor commands.

&error_exit is okay exactly when and where exit(1) is okay: on
configuration error during startup, and on certain unrecoverable errors
where it's unsafe to continue.

Likewise, &error_abort is okay exactly when and where abort() is okay:
mostly on programming errors.  Possibly also on "unexpected"
unrecoverable errors where it's unsafe to continue; we did that for most
memory allocation failures at some time, not sure we still do.

Perhaps we need to advise on proper use of abort() and exit(1), but I
doubt error.h is the right place to do it.

Perhaps we need to remind users some more that &error_exit is just like
exit(1) and &error_abort is just like abort().  Doubtful.

If we want to, I'd recommend a separate patch.

>  
> +/*
> + * Pass to error_setg() & friends to immediately show an error as a warning.
> + */
> +extern Error *error_warn;
> +
>  #endif
> diff --git a/util/error.c b/util/error.c
> index 80c89a2..85170e54 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -15,6 +15,7 @@
>  #include "qemu-common.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> +#include "qemu/timer.h"
>  
>  struct Error
>  {
> @@ -27,8 +28,9 @@ struct Error
>  
>  Error *error_abort;
>  Error *error_fatal;
> +Error *error_warn;
>  
> -static void error_handle_fatal(Error **errp, Error *err)
> +static bool error_handle_fatal(Error **errp, Error *err)
>  {
>      if (errp == &error_abort) {
>          fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> @@ -40,6 +42,20 @@ static void error_handle_fatal(Error **errp, Error *err)
>          error_report_err(err);
>          exit(1);
>      }
> +
> +    if (errp == &error_warn) {
> +        /* cannot use error_report_err() because it adds newlines */
> +        error_report("warning: [%s() at %s:%d] %s",
> +                     err->func, err->src, err->line, err->msg);

I don't think we should show err->func & friends.

We do for &error_abort, because that's a message for developers.

We don't for &error_exit, because that's for users.

> +        if (err->hint) {
> +            error_printf_unless_qmp("warning: [%s() at %s:%d] %s",
> +                                    err->func, err->src, err->line,
> +                                    err->hint->str);
> +        }
> +        return true;
> +    } else {
> +        return false;
> +    }
>  }

The function's name is now misleading.

>  
>  static void error_setv(Error **errp,
> @@ -61,10 +77,10 @@ static void error_setv(Error **errp,
>      err->line = line;
>      err->func = func;
>  
> -    error_handle_fatal(errp, err);
> -    *errp = err;
> -
> -    errno = saved_errno;
> +    if (!error_handle_fatal(errp, err)) {
> +        *errp = err;
> +        errno = saved_errno;
> +    }
>  }

Awkward.

>  
>  void error_set_internal(Error **errp,
> @@ -232,10 +248,11 @@ void error_propagate(Error **dst_errp, Error *local_err)
>      if (!local_err) {
>          return;
>      }
> -    error_handle_fatal(dst_errp, local_err);
> -    if (dst_errp && !*dst_errp) {
> -        *dst_errp = local_err;
> -    } else {
> -        error_free(local_err);
> +    if (!error_handle_fatal(dst_errp, local_err)) {
> +        if (dst_errp && !*dst_errp) {
> +            *dst_errp = local_err;
> +        } else {
> +            error_free(local_err);
> +        }
>      }
>  }

Just as awkward.