qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]