Markus Armbruster writes:
> Lluís Vilanova <vilan...@ac.upc.edu> 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 <vilan...@ac.upc.edu>
>> ---
>> 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, *@errp 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 *@dst_errp.
>> @@ -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
