[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] [doc] Introduce coding style for errors
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH] [doc] Introduce coding style for errors |
Date: |
Mon, 23 Nov 2015 18:48:34 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake writes:
> On 11/23/2015 07:18 AM, Lluís Vilanova wrote:
>> Gives some general guidelines for reporting errors in QEMU.
> This is more than just a doc patch; you are actually proposing new code
> in the way of &error_now. The commit message must absolutely mention
> changes like this, or possibly even split this into two changes
> (document existing behavior, then add new code and its matching docs).
Yes, sorry. I added it after writing most of the text, and forgot to update the
patch description. Will resend a fixed version.
[...]
>> +++ b/HACKING
>> @@ -157,3 +157,55 @@ painful. These are:
>> * you may assume that integers are 2s complement representation
>> * you may assume that right shift of a signed integer duplicates
>> the sign bit (ie it is an arithmetic shift, not a logical shift)
>> +
>> +7. Error reporting
>> +
>> +QEMU provides two different mechanisms for reporting errors. You should use
>> one
>> +of these mechanisms instead of manually reporting them (i.e., do not use
>> +'printf', 'exit' or 'abort').
> There are places where calling exit() is okay (when parsing command-line
> arguments), and where abort() is okay (in code that cannot be reached,
> such as a dead switch() branch). So I don't know if this sentence
> should be reworded, or if adding even something as simple as "generally"
> will weaken it too much.
Wouldn't it be better to use "error_setg(&error_fatal, ...)" instead of an exit?
Same applies to 'abort' and 'error_abort'; it additionally provides a message
plus a bit of of information on the failed function/file/line.
[...]
>> +
>> +For convenience, you can also use 'error_setg_errno' and 'error_setg_win32'
>> to
>> +append a message for OS-specific errors, and 'error_setg_file_open' for
>> errors
>> +when opening files.
> I'm worried that we may be trying to duplicate too much information, in
> which case one will get out of sync. Merely pointing to error.h may be
> good enough for the purposes of HACKING, rather than calling out all the
> subtleties (leaving that for error.h).
Aha. I can simply reference the three special error objects, and leave the rest
to error.h.
>>
>> +/*
>> + * Pass to error_setg() & friends to immediately show an error.
>> + *
>> + * Has the same formatting of #error_abort, but does not terminate QEMU.
> Doesn't that make it useful only for warnings? If so, should it
> automatically prepend "warning: "? Where do you propose using it, for a
> demonstration of why it is useful?
Actually, this new object caters to some comments I received, where guest and
device code should never terminate QEMU. To me it sounds like a non-terminating
abort, but I can certainly rename to object to 'errror_warn' and prepend the
appropriate "warning: " text.
>> +++ b/util/error.c
>> @@ -27,6 +27,7 @@ struct Error
>>
>> Error *error_abort;
>> Error *error_fatal;
>> +Error *error_now;
>>
>> static void error_handle_fatal(Error **errp, Error *err)
>> {
>> @@ -62,9 +63,14 @@ static void error_setv(Error **errp,
err-> func = func;
>>
>> error_handle_fatal(errp, err);
>> - *errp = err;
>> -
>> - errno = saved_errno;
>> + if (errp == &error_now) {
>> + fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>> + err->func, err->src, err->line);
>> + error_report_err(err);
> This lacks timestamping information on the "Unexpected error..." output;
> is that going to be a problem?
The code mimics 'error_handle_fatal', which does not show any timestamp.
Thanks,
Lluis
--
"And it's much the same thing with knowledge, for whenever you learn
something new, the whole world becomes that much richer."
-- The Princess of Pure Reason, as told by Norton Juster in The Phantom
Tollbooth