[Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Gives some general guidelines for reporting errors in QEMU. Signed-off-by: Lluís Vilanova --- HACKING | 36 1 file changed, 36 insertions(+) diff --git a/HACKING b/HACKING index 12fbc8a..1523bad 100644 --- a/HACKING +++ b/HACKING @@ -157,3 +157,39 @@ 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()'). + +7.1. Errors in user inputs + +QEMU provides the functions in "include/qemu/error-report.h" to report errors +related to inputs provided by the user (e.g., command line arguments or +configuration files). + +These functions generate error messages with a uniform format that can reference +a location on the offending input. + +7.2. Other errors + +QEMU provides the functions in "include/qapi/error.h" to report other types of +errors (i.e., not triggered by command line arguments or configuration files). + +Functions in this header are used to accumulate error messages in an 'Error' +object, which can be propagated up the call chain where it is finally reported. + +In its simplest form, you can immediately report an error with: + +error_setg(&error_fatal, "Error with %s", "arguments"); + +See the "include/qapi/error.h" header for additional convenience functions and +special arguments. Specially, see 'error_fatal' and 'error_abort' to show errors +and immediately terminate QEMU. + +WARNING: Do *not* use 'error_fatal' or 'error_abort' 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. Instead, you should use the 'error_report()' routine.
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
On 20.01.2016 15:10, Lluís Vilanova wrote:
> Thomas Huth writes:
>
>> On 18.01.2016 21:26, Eric Blake wrote:
>>> On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by: Lluís Vilanova
---
HACKING | 36
1 file changed, 36 insertions(+)
>> ...
+Functions in this header are used to accumulate error messages in an
'Error'
+object, which can be propagated up the call chain where it is finally
reported.
+
+In its simplest form, you can immediately report an error with:
+
+error_setg(&error_fatal, "Error with %s", "arguments");
>>>
>>> This paradigm doesn't appear anywhere in the current code base
>>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
>>> nothing directly passes error_fatal). It's a bit odd to document
>>> something that isn't actually used.
>
>> +1 for _not_ documenting this here: IMHO this looks ugly. If we want
>> something like this, I think we should introduce a proper
>> error_report_fatal() function instead.
>
> That's a bit of a chicken and egg problem. My main intention was to provide a
> best practices summary on reporting messages/errors, since QEMU's code is
> really
> heterogeneous on that regard. But there seems to be no consensus on some
> details
> of what the proper way should be with the current interfaces.
>
> Utility functions for "regular messages", warnings, fatals and aborts would
> definitiely be an improvement IMHO, but I dont have time to adapt existing
> code
> to these (and I was told not to add unused utility functions for this).
>
> Now, if I were able to add such functions, it'd be something like:
>
> // Generate message "as is"; not sure if this should exist.
> message_report(fmt, ...)
Not sure what this should be good for? We've already got error_report()
that generates messages "as is", haven't we?
> // Generate message with prepended file/line information for the caller.
> // Calls exit/abort on the last two.
> error_report_{warn,fatal,abort}(fmt, ...)
>
> // Same with an added message from strerror.
> error_report_{warn,fatal,abort}_errno(fmt, ...)
>
> But, should I add these without providing extensive patches that refactor code
> to use them?
Maybe create a patch that introduces the _fatal and _abort functions
(I'd skip the _warn functions for now), and use them in one or two files
(e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should
not be that much of work, and could be a good base for further discussion?
Thomas
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Thomas Huth writes:
> On 20.01.2016 15:10, Lluís Vilanova wrote:
>> Thomas Huth writes:
>>
>>> On 18.01.2016 21:26, Eric Blake wrote:
On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
> Gives some general guidelines for reporting errors in QEMU.
>
> Signed-off-by: Lluís Vilanova
> ---
> HACKING | 36
> 1 file changed, 36 insertions(+)
>>> ...
> +Functions in this header are used to accumulate error messages in an
> 'Error'
> +object, which can be propagated up the call chain where it is finally
> reported.
> +
> +In its simplest form, you can immediately report an error with:
> +
> +error_setg(&error_fatal, "Error with %s", "arguments");
This paradigm doesn't appear anywhere in the current code base
(hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
nothing directly passes error_fatal). It's a bit odd to document
something that isn't actually used.
>>
>>> +1 for _not_ documenting this here: IMHO this looks ugly. If we want
>>> something like this, I think we should introduce a proper
>>> error_report_fatal() function instead.
>>
>> That's a bit of a chicken and egg problem. My main intention was to provide a
>> best practices summary on reporting messages/errors, since QEMU's code is
>> really
>> heterogeneous on that regard. But there seems to be no consensus on some
>> details
>> of what the proper way should be with the current interfaces.
>>
>> Utility functions for "regular messages", warnings, fatals and aborts would
>> definitiely be an improvement IMHO, but I dont have time to adapt existing
>> code
>> to these (and I was told not to add unused utility functions for this).
>>
>> Now, if I were able to add such functions, it'd be something like:
>>
>> // Generate message "as is"; not sure if this should exist.
>> message_report(fmt, ...)
> Not sure what this should be good for? We've already got error_report()
> that generates messages "as is", haven't we?
Well, it just seemed wrong to me using error_report() to report "regular
messages" :)
>> // Generate message with prepended file/line information for the caller.
>> // Calls exit/abort on the last two.
>> error_report_{warn,fatal,abort}(fmt, ...)
>>
>> // Same with an added message from strerror.
>> error_report_{warn,fatal,abort}_errno(fmt, ...)
>>
>> But, should I add these without providing extensive patches that refactor
>> code
>> to use them?
> Maybe create a patch that introduces the _fatal and _abort functions
> (I'd skip the _warn functions for now), and use them in one or two files
> (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should
> not be that much of work, and could be a good base for further discussion?
I can do that. But then should 'error_fatal' and 'error_abort' be officially
deprecated in favour of error_report_fatal() and error_report_abort()?
Cheers,
Lluis
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Lluís Vilanova writes:
> Thomas Huth writes:
>> On 20.01.2016 15:10, Lluís Vilanova wrote:
>>> Thomas Huth writes:
>>>
On 18.01.2016 21:26, Eric Blake wrote:
> On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
>> Gives some general guidelines for reporting errors in QEMU.
>>
>> Signed-off-by: Lluís Vilanova
>> ---
>> HACKING | 36
>> 1 file changed, 36 insertions(+)
...
>> +Functions in this header are used to accumulate error messages in an
>> 'Error'
>> +object, which can be propagated up the call chain where it is finally
>> reported.
>> +
>> +In its simplest form, you can immediately report an error with:
>> +
>> +error_setg(&error_fatal, "Error with %s", "arguments");
>
> This paradigm doesn't appear anywhere in the current code base
> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
> nothing directly passes error_fatal). It's a bit odd to document
> something that isn't actually used.
>>>
+1 for _not_ documenting this here: IMHO this looks ugly. If we want
something like this, I think we should introduce a proper
error_report_fatal() function instead.
>>>
>>> That's a bit of a chicken and egg problem. My main intention was to provide
>>> a
>>> best practices summary on reporting messages/errors, since QEMU's code is
>>> really
>>> heterogeneous on that regard. But there seems to be no consensus on some
>>> details
>>> of what the proper way should be with the current interfaces.
>>>
>>> Utility functions for "regular messages", warnings, fatals and aborts would
>>> definitiely be an improvement IMHO, but I dont have time to adapt existing
>>> code
>>> to these (and I was told not to add unused utility functions for this).
>>>
>>> Now, if I were able to add such functions, it'd be something like:
>>>
>>> // Generate message "as is"; not sure if this should exist.
>>> message_report(fmt, ...)
>> Not sure what this should be good for? We've already got error_report()
>> that generates messages "as is", haven't we?
> Well, it just seemed wrong to me using error_report() to report "regular
> messages" :)
>>> // Generate message with prepended file/line information for the caller.
>>> // Calls exit/abort on the last two.
>>> error_report_{warn,fatal,abort}(fmt, ...)
>>>
>>> // Same with an added message from strerror.
>>> error_report_{warn,fatal,abort}_errno(fmt, ...)
>>>
>>> But, should I add these without providing extensive patches that refactor
>>> code
>>> to use them?
>> Maybe create a patch that introduces the _fatal and _abort functions
>> (I'd skip the _warn functions for now), and use them in one or two files
>> (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should
>> not be that much of work, and could be a good base for further discussion?
> I can do that. But then should 'error_fatal' and 'error_abort' be officially
> deprecated in favour of error_report_fatal() and error_report_abort()?
Sorry, I see this is misleading. I mean deprecate directly using
"error_setg(error_fatal)"; you can still decide to pass error_fatal as an error
object to other user functions.
Cheers,
Lluis
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
On 27.01.2016 20:20, Lluís Vilanova wrote:
> Lluís Vilanova writes:
>
>> Thomas Huth writes:
>>> On 20.01.2016 15:10, Lluís Vilanova wrote:
Thomas Huth writes:
> On 18.01.2016 21:26, Eric Blake wrote:
>> On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
>>> Gives some general guidelines for reporting errors in QEMU.
>>>
>>> Signed-off-by: Lluís Vilanova
>>> ---
>>> HACKING | 36
>>> 1 file changed, 36 insertions(+)
> ...
>>> +Functions in this header are used to accumulate error messages in an
>>> 'Error'
>>> +object, which can be propagated up the call chain where it is finally
>>> reported.
>>> +
>>> +In its simplest form, you can immediately report an error with:
>>> +
>>> +error_setg(&error_fatal, "Error with %s", "arguments");
>>
>> This paradigm doesn't appear anywhere in the current code base
>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
>> nothing directly passes error_fatal). It's a bit odd to document
>> something that isn't actually used.
> +1 for _not_ documenting this here: IMHO this looks ugly. If we want
> something like this, I think we should introduce a proper
> error_report_fatal() function instead.
That's a bit of a chicken and egg problem. My main intention was to
provide a
best practices summary on reporting messages/errors, since QEMU's code is
really
heterogeneous on that regard. But there seems to be no consensus on some
details
of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would
definitiely be an improvement IMHO, but I dont have time to adapt existing
code
to these (and I was told not to add unused utility functions for this).
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)
>
>>> Not sure what this should be good for? We've already got error_report()
>>> that generates messages "as is", haven't we?
>
>> Well, it just seemed wrong to me using error_report() to report "regular
>> messages" :)
>
>
// Generate message with prepended file/line information for the caller.
// Calls exit/abort on the last two.
error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)
But, should I add these without providing extensive patches that refactor
code
to use them?
>
>>> Maybe create a patch that introduces the _fatal and _abort functions
>>> (I'd skip the _warn functions for now), and use them in one or two files
>>> (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should
>>> not be that much of work, and could be a good base for further discussion?
>
>> I can do that. But then should 'error_fatal' and 'error_abort' be officially
>> deprecated in favour of error_report_fatal() and error_report_abort()?
>
> Sorry, I see this is misleading. I mean deprecate directly using
> "error_setg(error_fatal)"; you can still decide to pass error_fatal as an
> error
> object to other user functions.
Since we hardly got any of these in the code right now, I don't see an
urgent need to explicitely say that this should be deprecated. I hope
that people rather will use the new functions automatically instead
since these sounds much more intuitive, IMHO.
Thomas
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Thomas Huth writes:
> On 27.01.2016 20:20, Lluís Vilanova wrote:
>> Lluís Vilanova writes:
>>
>>> Thomas Huth writes:
On 20.01.2016 15:10, Lluís Vilanova wrote:
> Thomas Huth writes:
>
>> On 18.01.2016 21:26, Eric Blake wrote:
>>> On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by: Lluís Vilanova
---
HACKING | 36
1 file changed, 36 insertions(+)
>> ...
+Functions in this header are used to accumulate error messages in an
'Error'
+object, which can be propagated up the call chain where it is finally
reported.
+
+In its simplest form, you can immediately report an error with:
+
+error_setg(&error_fatal, "Error with %s", "arguments");
>>>
>>> This paradigm doesn't appear anywhere in the current code base
>>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
>>> nothing directly passes error_fatal). It's a bit odd to document
>>> something that isn't actually used.
>
>> +1 for _not_ documenting this here: IMHO this looks ugly. If we want
>> something like this, I think we should introduce a proper
>> error_report_fatal() function instead.
>
> That's a bit of a chicken and egg problem. My main intention was to
> provide a
> best practices summary on reporting messages/errors, since QEMU's code is
> really
> heterogeneous on that regard. But there seems to be no consensus on some
> details
> of what the proper way should be with the current interfaces.
>
> Utility functions for "regular messages", warnings, fatals and aborts
> would
> definitiely be an improvement IMHO, but I dont have time to adapt
> existing code
> to these (and I was told not to add unused utility functions for this).
>
> Now, if I were able to add such functions, it'd be something like:
>
> // Generate message "as is"; not sure if this should exist.
> message_report(fmt, ...)
>>
Not sure what this should be good for? We've already got error_report()
that generates messages "as is", haven't we?
>>
>>> Well, it just seemed wrong to me using error_report() to report "regular
>>> messages" :)
>>
>>
> // Generate message with prepended file/line information for the caller.
> // Calls exit/abort on the last two.
> error_report_{warn,fatal,abort}(fmt, ...)
>
> // Same with an added message from strerror.
> error_report_{warn,fatal,abort}_errno(fmt, ...)
>
> But, should I add these without providing extensive patches that refactor
> code
> to use them?
>>
Maybe create a patch that introduces the _fatal and _abort functions
(I'd skip the _warn functions for now), and use them in one or two files
(e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should
not be that much of work, and could be a good base for further discussion?
>>
>>> I can do that. But then should 'error_fatal' and 'error_abort' be officially
>>> deprecated in favour of error_report_fatal() and error_report_abort()?
>>
>> Sorry, I see this is misleading. I mean deprecate directly using
>> "error_setg(error_fatal)"; you can still decide to pass error_fatal as an
>> error
>> object to other user functions.
> Since we hardly got any of these in the code right now, I don't see an
> urgent need to explicitely say that this should be deprecated. I hope
> that people rather will use the new functions automatically instead
> since these sounds much more intuitive, IMHO.
Got it. I'm writing some of them now.
Cheers,
Lluis
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
On 01/15/2016 06:54 AM, Lluís Vilanova wrote: > Gives some general guidelines for reporting errors in QEMU. > > Signed-off-by: Lluís Vilanova > --- > HACKING | 36 > 1 file changed, 36 insertions(+) > > +7.1. Errors in user inputs > + > +QEMU provides the functions in "include/qemu/error-report.h" to report errors > +related to inputs provided by the user (e.g., command line arguments or > +configuration files). > + > +These functions generate error messages with a uniform format that can > reference > +a location on the offending input. s/on/in/ > + > +7.2. Other errors > + > +QEMU provides the functions in "include/qapi/error.h" to report other types > of > +errors (i.e., not triggered by command line arguments or configuration > files). Maybe: "not directly triggered". After all, we DO have places where Error is used which can ultimately be traced to a user command (such as in QMP commands), but where the local code can also be called internally; the use of Error at the local level then lets us leave it up to the caller whether to report a message (because the caller has more context). > + > +Functions in this header are used to accumulate error messages in an 'Error' > +object, which can be propagated up the call chain where it is finally > reported. > + > +In its simplest form, you can immediately report an error with: > + > +error_setg(&error_fatal, "Error with %s", "arguments"); This paradigm doesn't appear anywhere in the current code base (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but nothing directly passes error_fatal). It's a bit odd to document something that isn't actually used. > + > +See the "include/qapi/error.h" header for additional convenience functions > and > +special arguments. Specially, see 'error_fatal' and 'error_abort' to show > errors s/Specially/Specifically/ > +and immediately terminate QEMU. > + > +WARNING: Do *not* use 'error_fatal' or 'error_abort' for errors that are (or > can > +be) triggered by guest code (e.g., some unimplimented corner case in guest > code s/unimplimented/unimplemented/ > +translation or device code). Otherwise that can be abused by guest code to > +terminate QEMU. Instead, you should use the 'error_report()' routine. But a definite improvement over v2. I think we're getting closer to a good summary. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
On 18.01.2016 21:26, Eric Blake wrote: > On 01/15/2016 06:54 AM, Lluís Vilanova wrote: >> Gives some general guidelines for reporting errors in QEMU. >> >> Signed-off-by: Lluís Vilanova >> --- >> HACKING | 36 >> 1 file changed, 36 insertions(+) ... >> +Functions in this header are used to accumulate error messages in an 'Error' >> +object, which can be propagated up the call chain where it is finally >> reported. >> + >> +In its simplest form, you can immediately report an error with: >> + >> +error_setg(&error_fatal, "Error with %s", "arguments"); > > This paradigm doesn't appear anywhere in the current code base > (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but > nothing directly passes error_fatal). It's a bit odd to document > something that isn't actually used. +1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper error_report_fatal() function instead. Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Thomas Huth writes:
> On 18.01.2016 21:26, Eric Blake wrote:
>> On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
>>> Gives some general guidelines for reporting errors in QEMU.
>>>
>>> Signed-off-by: Lluís Vilanova
>>> ---
>>> HACKING | 36
>>> 1 file changed, 36 insertions(+)
> ...
>>> +Functions in this header are used to accumulate error messages in an
>>> 'Error'
>>> +object, which can be propagated up the call chain where it is finally
>>> reported.
>>> +
>>> +In its simplest form, you can immediately report an error with:
>>> +
>>> +error_setg(&error_fatal, "Error with %s", "arguments");
>>
>> This paradigm doesn't appear anywhere in the current code base
>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
>> nothing directly passes error_fatal). It's a bit odd to document
>> something that isn't actually used.
> +1 for _not_ documenting this here: IMHO this looks ugly. If we want
> something like this, I think we should introduce a proper
> error_report_fatal() function instead.
That's a bit of a chicken and egg problem. My main intention was to provide a
best practices summary on reporting messages/errors, since QEMU's code is really
heterogeneous on that regard. But there seems to be no consensus on some details
of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would
definitiely be an improvement IMHO, but I dont have time to adapt existing code
to these (and I was told not to add unused utility functions for this).
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)
// Generate message with prepended file/line information for the caller.
// Calls exit/abort on the last two.
error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)
But, should I add these without providing extensive patches that refactor code
to use them?
Cheers,
Lluis
