Re: [Qemu-devel] Coding style for errors
On 23/10/15 19:02, Dr. David Alan Gilbert wrote: > * Stefan Hajnoczi (stefa...@gmail.com) wrote: >> On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote: >>> Markus Armbruster writes: >>> Lluís Vilanovawrites: >>> [...] > 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
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 Vilanovawrites: > >> [...] > >> >> 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
Re: [Qemu-devel] Coding style for errors
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 Vilanovawrites: >> [...] 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
On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote: > Markus Armbruster writes: > > > Lluís Vilanovawrites: > [...] > >> 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
* Stefan Hajnoczi (stefa...@gmail.com) wrote: > On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote: > > Markus Armbruster writes: > > > > > Lluís Vilanovawrites: > > [...] > > >> 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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] Coding style for errors
Stefan Hajnoczi writes: > On Thu, Oct 22, 2015 at 03:30:34PM +0200, Lluís Vilanova wrote: >> Markus Armbruster writes: >> >> > Lluís Vilanovawrites: >> [...] >> >> 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
Markus Armbruster writes: > Lluís Vilanovawrites: [...] >> 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(_fatal, msg, ...)' Reports a fatal error with the given error message and exits QEMU. * 'error_setg(_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
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(..., _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 signature.asc Description: OpenPGP digital signature
[Qemu-devel] Coding style for errors
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
On 21 October 2015 at 17:48, Markus Armbrusterwrote: > Lluís Vilanova 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 Vilanovawrites: > 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: _fatal. Likewise, for programming errors: _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
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(..., _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