[Qemu-devel] Coding style for errors

[Qemu-devel] Coding style for errors

[Lluís Vilanova]

Hi,

I was wondering what is the proper way (or ways, depending on the subsystem) of
reporting and signalling errors in QEMU. The coding style file does not seem to
mention it, and the code uses all kinds of forms for that:

* printf + exit(1)
* fprintf(stderr) + exit(1)
* error_report + exit(1)
* cpu_abort
* Some other I probably forgot

So, is there any agreement on what should be used? If so, could that please be
added to CODING_STYLE?


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

Re: [Qemu-devel] Coding style for errors

[Eric Blake]

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

On 10/21/2015 09:03 AM, Lluís Vilanova wrote:
> Hi,
> 
> I was wondering what is the proper way (or ways, depending on the subsystem) of
> reporting and signalling errors in QEMU. The coding style file does not seem to
> mention it, and the code uses all kinds of forms for that:
> 
> * printf + exit(1)
> * fprintf(stderr) + exit(1)

Existing code doesn't all have to be switched, but new code...

> * error_report + exit(1)

...should favor this approach, or even:

error_setg(..., &error_fatal)

as shorthand.

> * cpu_abort
> * Some other I probably forgot
> 
> So, is there any agreement on what should be used? If so, could that please be
> added to CODING_STYLE?

include/qapi/error.h has more documentation on how to best use struct
Error and the various error_* functions, but you're right that a blurb
in CODING_STYLE can't hurt. Would you care to try writing a first draft?

-- 
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 --]

Re: [Qemu-devel] Coding style for errors

[Lluís Vilanova]

Eric Blake writes:

> On 10/21/2015 09:03 AM, Lluís Vilanova wrote:
>> Hi,
>> 
>> I was wondering what is the proper way (or ways, depending on the subsystem) of
>> reporting and signalling errors in QEMU. The coding style file does not seem to
>> mention it, and the code uses all kinds of forms for that:
>> 
>> * printf + exit(1)
>> * fprintf(stderr) + exit(1)

> Existing code doesn't all have to be switched, but new code...

>> * error_report + exit(1)

> ...should favor this approach, or even:

> error_setg(..., &error_fatal)

> as shorthand.

Aha! That's what I was looking for, which provides both exit and abort :)


>> * cpu_abort
>> * Some other I probably forgot
>> 
>> So, is there any agreement on what should be used? If so, could that please be
>> added to CODING_STYLE?

> include/qapi/error.h has more documentation on how to best use struct
> Error and the various error_* functions, but you're right that a blurb
> in CODING_STYLE can't hurt. Would you care to try writing a first draft?

I'll try to give it a swirl tomorrow.


Cheers,
  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

Re: [Qemu-devel] Coding style for errors

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Hi,
>
> I was wondering what is the proper way (or ways, depending on the subsystem) of
> reporting and signalling errors in QEMU. The coding style file does not seem to
> mention it, and the code uses all kinds of forms for that:
>
> * printf + exit(1)
> * fprintf(stderr) + exit(1)
> * error_report + exit(1)
> * cpu_abort
> * Some other I probably forgot

cpu_abort() and hw_error() are fancy ways to abort().  Terminating with
abort() on "this can't be happening" conditions is perfectly sensible,
and doing it in fancy ways can be useful.  For other errors, it's
inappropriate.

qemu/error-report.h is for reporting errors to the user.  Why not simply
fprintf(stderr, ...)?  Several reasons:

* error_report() & friends report errors in a uniform format.

* They do the right thing inside monitor commands: report the error to
  the monitor instead of stderr.

* They can add location information.

* They can add timestamps (-msg timestamp=on).

* If we ever do proper logging, they'll log the error.

There are many places left that fprintf().  Please don't add more.

qapi/error.h is for propagating errors up the call chain.  At some
point, you'll either recover and throw away the error, or you report it.
Convenience function error_report_err() makes that easy, but it's really
just a thin wrapper around error_report().

Another convenience feature makes reporting *fatal* errors easy:
&error_fatal.  Likewise, for programming errors: &error_abort.

When a simpler method for reporting success/failure to the caller
suffices, it's perfectly fine to use it.  E.g. returning a valid pointer
on success and null pointer on failure, or non-negative integer on
success and negative errno code on failure.

> So, is there any agreement on what should be used? If so, could that please be
> added to CODING_STYLE?

I think HACKING would be a better fit.

Re: [Qemu-devel] Coding style for errors

[Peter Maydell]

On 21 October 2015 at 17:48, Markus Armbruster <armbru@redhat.com> wrote:
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>
>> Hi,
>>
>> I was wondering what is the proper way (or ways, depending on the subsystem) of
>> reporting and signalling errors in QEMU. The coding style file does not seem to
>> mention it, and the code uses all kinds of forms for that:
>>
>> * printf + exit(1)
>> * fprintf(stderr) + exit(1)
>> * error_report + exit(1)
>> * cpu_abort
>> * Some other I probably forgot
>
> cpu_abort() and hw_error() are fancy ways to abort().  Terminating with
> abort() on "this can't be happening" conditions is perfectly sensible,
> and doing it in fancy ways can be useful.  For other errors, it's
> inappropriate.

In particular, the fact that TCG will cpu_abort() if you try to
generate code out of something that's not RAM is a perpetual
source of confusion to users, because the usual reason it happens
is not a QEMU bug but buggy guest code jumping off to a random address...

-- PMM

Re: [Qemu-devel] Coding style for errors

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
[...]
>> So, is there any agreement on what should be used? If so, could that please be
>> added to CODING_STYLE?

> I think HACKING would be a better fit.

What about this? (at the end of HACKING) Feel free to add references to other
functions you think are important. I'll send a patch once we agree on the text.

Cheers,
  Lluis


7. Error reporting

QEMU provides two different mechanisms for reporting errors. You should use one
of these mechanisms instead of manually reporting them (e.g., 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.

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.

7.3. Errors with an immediate exit/abort

There are two convenience forms to report errors in a way that immediately
terminates QEMU:

* 'error_setg(&error_fatal, msg, ...)'

  Reports a fatal error with the given error message and exits QEMU.

* 'error_setg(&error_abort, msg, ...)'

  Reports a programming error with the given error message and aborts QEMU.

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.


-- 
"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

Re: [Qemu-devel] Coding style for errors

[Stefan Hajnoczi]

On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote:
> Markus Armbruster writes:
> 
> > Lluís Vilanova <vilanova@ac.upc.edu> writes:
> [...]
> >> So, is there any agreement on what should be used? If so, could that please be
> >> added to CODING_STYLE?
> 
> > I think HACKING would be a better fit.
> 
> What about this? (at the end of HACKING) Feel free to add references to other
> functions you think are important. I'll send a patch once we agree on the text.
> 
> Cheers,
>   Lluis
> 
> 
> 7. Error reporting

Guest-triggerable errors should not terminate QEMU.  There are plently
of examples where this is violated today but there are good reasons to
stop doing it.

Denial of service cases:

1. If a guest userspace application is somehow able to trigger a QEMU
   abort, then an unprivileged guest application is able to bring down
   the whole VM.

2. If nested virtualization is used, it's possible that a nested guest
   can kill its parent, and thereby also kill its sibling VMs.

3. abort(3) is heavyweight if crash reporting/coredumps are enabled.  A
   broken/malicious guest that keeps triggering abort(3) can be a big
   nuisance that consumes memory, disk, and CPU resources.

Emulated hardware should behave the same way that physical hardware
behaves.  This may mean that the device becomes non-operational (ignores
or fails new requests) until the next hard or soft reset.

Re: [Qemu-devel] Coding style for errors

[Laszlo Ersek]

On 10/23/15 18:01, Stefan Hajnoczi wrote:
> On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote:
>> Markus Armbruster writes:
>>
>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> [...]
>>>> So, is there any agreement on what should be used? If so, could that please be
>>>> added to CODING_STYLE?
>>
>>> I think HACKING would be a better fit.
>>
>> What about this? (at the end of HACKING) Feel free to add references to other
>> functions you think are important. I'll send a patch once we agree on the text.
>>
>> Cheers,
>>   Lluis
>>
>>
>> 7. Error reporting
> 
> Guest-triggerable errors should not terminate QEMU.  There are plently
> of examples where this is violated today but there are good reasons to
> stop doing it.
> 
> Denial of service cases:
> 
> 1. If a guest userspace application is somehow able to trigger a QEMU
>    abort, then an unprivileged guest application is able to bring down
>    the whole VM.
> 
> 2. If nested virtualization is used, it's possible that a nested guest
>    can kill its parent, and thereby also kill its sibling VMs.
> 
> 3. abort(3) is heavyweight if crash reporting/coredumps are enabled.  A
>    broken/malicious guest that keeps triggering abort(3) can be a big
>    nuisance that consumes memory, disk, and CPU resources.
> 
> Emulated hardware should behave the same way that physical hardware
> behaves.

I thought that's what we have now; for example, a buggy video driver can
lock up or crash the entire box. Faithful emulation FTW! ;)

> This may mean that the device becomes non-operational (ignores
> or fails new requests) until the next hard or soft reset.
> 

Re: [Qemu-devel] Coding style for errors

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@gmail.com) wrote:
> On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote:
> > Markus Armbruster writes:
> > 
> > > Lluís Vilanova <vilanova@ac.upc.edu> writes:
> > [...]
> > >> So, is there any agreement on what should be used? If so, could that please be
> > >> added to CODING_STYLE?
> > 
> > > I think HACKING would be a better fit.
> > 
> > What about this? (at the end of HACKING) Feel free to add references to other
> > functions you think are important. I'll send a patch once we agree on the text.
> > 
> > Cheers,
> >   Lluis
> > 
> > 
> > 7. Error reporting
> 
> Guest-triggerable errors should not terminate QEMU.  There are plently
> of examples where this is violated today but there are good reasons to
> stop doing it.
> 
> Denial of service cases:
> 
> 1. If a guest userspace application is somehow able to trigger a QEMU
>    abort, then an unprivileged guest application is able to bring down
>    the whole VM.
> 
> 2. If nested virtualization is used, it's possible that a nested guest
>    can kill its parent, and thereby also kill its sibling VMs.
> 
> 3. abort(3) is heavyweight if crash reporting/coredumps are enabled.  A
>    broken/malicious guest that keeps triggering abort(3) can be a big
>    nuisance that consumes memory, disk, and CPU resources.
> 
> Emulated hardware should behave the same way that physical hardware
> behaves.  This may mean that the device becomes non-operational (ignores
> or fails new requests) until the next hard or soft reset.

I'd add that if the QEMU detects that the guest has done something really
stupid and that the device is now dead until reset, then it should
output something diagnostic into the logs; otherwise everyone just
blames qemu and says it stopped (and I mean log, not trace - I want
to see this in the type of thing a users sends so that we have some
idea where to look immediately).

Dave
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] Coding style for errors

[Thomas Huth]

On 23/10/15 19:02, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@gmail.com) wrote:
>> On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote:
>>> Markus Armbruster writes:
>>>
>>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> [...]
>>>>> So, is there any agreement on what should be used? If so, could that please be
>>>>> added to CODING_STYLE?
>>>
>>>> I think HACKING would be a better fit.
>>>
>>> What about this? (at the end of HACKING) Feel free to add references to other
>>> functions you think are important. I'll send a patch once we agree on the text.
>>>
>>> Cheers,
>>>   Lluis
>>>
>>>
>>> 7. Error reporting
>>
>> Guest-triggerable errors should not terminate QEMU.  There are plently
>> of examples where this is violated today but there are good reasons to
>> stop doing it.
>>
>> Denial of service cases:
>>
>> 1. If a guest userspace application is somehow able to trigger a QEMU
>>    abort, then an unprivileged guest application is able to bring down
>>    the whole VM.
>>
>> 2. If nested virtualization is used, it's possible that a nested guest
>>    can kill its parent, and thereby also kill its sibling VMs.
>>
>> 3. abort(3) is heavyweight if crash reporting/coredumps are enabled.  A
>>    broken/malicious guest that keeps triggering abort(3) can be a big
>>    nuisance that consumes memory, disk, and CPU resources.
>>
>> Emulated hardware should behave the same way that physical hardware
>> behaves.  This may mean that the device becomes non-operational (ignores
>> or fails new requests) until the next hard or soft reset.
> 
> I'd add that if the QEMU detects that the guest has done something really
> stupid and that the device is now dead until reset, then it should
> output something diagnostic into the logs; otherwise everyone just
> blames qemu and says it stopped (and I mean log, not trace - I want
> to see this in the type of thing a users sends so that we have some
> idea where to look immediately).

... and call qemu_system_guest_panicked() so that the management layers
can flag the guest accordingly?

 Thomas

Re: [Qemu-devel] Coding style for errors

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote:
>> Markus Armbruster writes:
>> 
>> > Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> [...]
>> >> So, is there any agreement on what should be used? If so, could that please be
>> >> added to CODING_STYLE?
>> 
>> > I think HACKING would be a better fit.
>> 
>> What about this? (at the end of HACKING) Feel free to add references to other
>> functions you think are important. I'll send a patch once we agree on the text.
>> 
>> Cheers,
>> Lluis
>> 
>> 
>> 7. Error reporting

> Guest-triggerable errors should not terminate QEMU.  There are plently
> of examples where this is violated today but there are good reasons to
> stop doing it.

> Denial of service cases:

> 1. If a guest userspace application is somehow able to trigger a QEMU
>    abort, then an unprivileged guest application is able to bring down
>    the whole VM.

> 2. If nested virtualization is used, it's possible that a nested guest
>    can kill its parent, and thereby also kill its sibling VMs.

> 3. abort(3) is heavyweight if crash reporting/coredumps are enabled.  A
>    broken/malicious guest that keeps triggering abort(3) can be a big
>    nuisance that consumes memory, disk, and CPU resources.

> Emulated hardware should behave the same way that physical hardware
> behaves.  This may mean that the device becomes non-operational (ignores
> or fails new requests) until the next hard or soft reset.

I'm not sure how this should be translated into the form of error-reporting
guidelines.


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

Re: [Qemu-devel] Coding style for errors

[Stefan Hajnoczi]

On Fri, Oct 23, 2015 at 07:34:50PM +0200, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
> > On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote:
> >> Markus Armbruster writes:
> >> 
> >> > Lluís Vilanova <vilanova@ac.upc.edu> writes:
> >> [...]
> >> >> So, is there any agreement on what should be used? If so, could that please be
> >> >> added to CODING_STYLE?
> >> 
> >> > I think HACKING would be a better fit.
> >> 
> >> What about this? (at the end of HACKING) Feel free to add references to other
> >> functions you think are important. I'll send a patch once we agree on the text.
> >> 
> >> Cheers,
> >> Lluis
> >> 
> >> 
> >> 7. Error reporting
> 
> > Guest-triggerable errors should not terminate QEMU.  There are plently
> > of examples where this is violated today but there are good reasons to
> > stop doing it.
> 
> > Denial of service cases:
> 
> > 1. If a guest userspace application is somehow able to trigger a QEMU
> >    abort, then an unprivileged guest application is able to bring down
> >    the whole VM.
> 
> > 2. If nested virtualization is used, it's possible that a nested guest
> >    can kill its parent, and thereby also kill its sibling VMs.
> 
> > 3. abort(3) is heavyweight if crash reporting/coredumps are enabled.  A
> >    broken/malicious guest that keeps triggering abort(3) can be a big
> >    nuisance that consumes memory, disk, and CPU resources.
> 
> > Emulated hardware should behave the same way that physical hardware
> > behaves.  This may mean that the device becomes non-operational (ignores
> > or fails new requests) until the next hard or soft reset.
> 
> I'm not sure how this should be translated into the form of error-reporting
> guidelines.

It needs to be close to the text you are adding since your text
mentioned immediate exits and abort(3).  People should be aware that
using those approaches can introduce security problems.

Stefan