qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [RFC][PATCH v3 ] utils: Improve and document error reporting
@ 2016-01-15 13:54 Lluís Vilanova
  2016-01-15 13:54 ` [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors Lluís Vilanova
  0 siblings, 1 reply; 10+ messages in thread
From: Lluís Vilanova @ 2016-01-15 13:54 UTC (permalink / raw)
  To: qemu-devel
  Cc: Stefan Hajnoczi, Thomas Huth, Dr . David Alan Gilbert, Markus Armbruster

Adds support for reporting non-terminating errors (dubbed warnings), and briefly
documents what routines should developers generally use to keep error reporting
consistent.

Changes in v3
=============

* Drop special object 'error_warn' in favour of raw 'error_report()'
  [suggested by Markus Armbruster].


Changes in v2
=============

* Split in two patches.
* Explicitly add a warning error object.


Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---

Lluís Vilanova (1):
      doc: Introduce coding style for errors


 HACKING |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)


To: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Eric Blake <eblake@redhat.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-15 13:54 [Qemu-devel] [RFC][PATCH v3 ] utils: Improve and document error reporting Lluís Vilanova
@ 2016-01-15 13:54 ` Lluís Vilanova
  2016-01-18 20:26   ` Eric Blake
  0 siblings, 1 reply; 10+ messages in thread
From: Lluís Vilanova @ 2016-01-15 13:54 UTC (permalink / raw)
  To: qemu-devel
  Cc: Stefan Hajnoczi, Thomas Huth, Dr . David Alan Gilbert, Markus Armbruster

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 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.

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-15 13:54 ` [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors Lluís Vilanova
@ 2016-01-18 20:26   ` Eric Blake
  2016-01-19  9:38     ` Thomas Huth
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2016-01-18 20:26 UTC (permalink / raw)
  To: Lluís Vilanova, qemu-devel
  Cc: Stefan Hajnoczi, Thomas Huth, Dr . David Alan Gilbert, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]

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 <vilanova@ac.upc.edu>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-18 20:26   ` Eric Blake
@ 2016-01-19  9:38     ` Thomas Huth
  2016-01-20 14:10       ` Lluís Vilanova
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2016-01-19  9:38 UTC (permalink / raw)
  To: Eric Blake, Lluís Vilanova, qemu-devel
  Cc: Stefan Hajnoczi, Dr . David Alan Gilbert, Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

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 <vilanova@ac.upc.edu>
>> ---
>>  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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-19  9:38     ` Thomas Huth
@ 2016-01-20 14:10       ` Lluís Vilanova
  2016-01-27 12:03         ` Thomas Huth
  0 siblings, 1 reply; 10+ messages in thread
From: Lluís Vilanova @ 2016-01-20 14:10 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Stefan Hajnoczi, qemu-devel, Markus Armbruster, Dr . David Alan Gilbert

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 <vilanova@ac.upc.edu>
>>> ---
>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-20 14:10       ` Lluís Vilanova
@ 2016-01-27 12:03         ` Thomas Huth
  2016-01-27 19:06           ` Lluís Vilanova
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2016-01-27 12:03 UTC (permalink / raw)
  To: Lluís Vilanova
  Cc: Stefan Hajnoczi, qemu-devel, Markus Armbruster, Dr . David Alan Gilbert

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 <vilanova@ac.upc.edu>
>>>> ---
>>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-27 12:03         ` Thomas Huth
@ 2016-01-27 19:06           ` Lluís Vilanova
  2016-01-27 19:20             ` Lluís Vilanova
  0 siblings, 1 reply; 10+ messages in thread
From: Lluís Vilanova @ 2016-01-27 19:06 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Stefan Hajnoczi, qemu-devel, Dr . David Alan Gilbert, Markus Armbruster

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 <vilanova@ac.upc.edu>
>>>>> ---
>>>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-27 19:06           ` Lluís Vilanova
@ 2016-01-27 19:20             ` Lluís Vilanova
  2016-01-28 14:27               ` Thomas Huth
  0 siblings, 1 reply; 10+ messages in thread
From: Lluís Vilanova @ 2016-01-27 19:20 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Stefan Hajnoczi, qemu-devel, Markus Armbruster, Dr . David Alan Gilbert

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 <vilanova@ac.upc.edu>
>>>>>> ---
>>>>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-27 19:20             ` Lluís Vilanova
@ 2016-01-28 14:27               ` Thomas Huth
  2016-01-28 18:31                 ` Lluís Vilanova
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2016-01-28 14:27 UTC (permalink / raw)
  To: Lluís Vilanova
  Cc: Stefan Hajnoczi, qemu-devel, Markus Armbruster, Dr . David Alan Gilbert

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 <vilanova@ac.upc.edu>
>>>>>>> ---
>>>>>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
  2016-01-28 14:27               ` Thomas Huth
@ 2016-01-28 18:31                 ` Lluís Vilanova
  0 siblings, 0 replies; 10+ messages in thread
From: Lluís Vilanova @ 2016-01-28 18:31 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Stefan Hajnoczi, qemu-devel, Dr . David Alan Gilbert, Markus Armbruster

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 <vilanova@ac.upc.edu>
>>>>>>>> ---
>>>>>>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-01-28 18:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-15 13:54 [Qemu-devel] [RFC][PATCH v3 ] utils: Improve and document error reporting Lluís Vilanova
2016-01-15 13:54 ` [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors Lluís Vilanova
2016-01-18 20:26   ` Eric Blake
2016-01-19  9:38     ` Thomas Huth
2016-01-20 14:10       ` Lluís Vilanova
2016-01-27 12:03         ` Thomas Huth
2016-01-27 19:06           ` Lluís Vilanova
2016-01-27 19:20             ` Lluís Vilanova
2016-01-28 14:27               ` Thomas Huth
2016-01-28 18:31                 ` Lluís Vilanova

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).