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

[Lluís Vilanova]

Markus Armbruster writes:

> 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.

Certainly true. The target was to avoid raw uses of the more verbose error_*
routines in the most common cases, concentrating most uses onto error_setg. If I
eliminate the new 'error_warn' then there is no clear and consistent formatting
substitute for printf.

Since I do not have time to port existing printf's into 'error_warn', I can just
drop this patch. Instead, we can simply admonish developers to use
'error_report' instead of plain 'printf' (although formatting will vary).

Also, using anything that is non-trivially more complex than a printf will
probably shy away developers from using it.


>> 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.
[...]

Hmm, then I'll remove those advices and merge your comments into HACKING (which
tries to provide very high-level guidelines of when it's safe to exit/abort). In
fact the advices on the comments are mere reminders of the text there.

Thanks,
  Lluis