| Message ID | cover.1386049636.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > Following our discussion RE self asserting API calls, here is a spin of > my proposal. This series obsoletes the need for _nofail variants for > Error ** accepting APIs. Is also greately reduces the verbosity of calls > sites that are currently asserting against errors. > > Patch 1 is the main event - addition of error_abort. The following > patches then cleanup uses of _nofail and assert_no_error(). > > To give it a smoke test, I introduce a (critical) bug into QOM: [...] > 32 files changed, 100 insertions(+), 143 deletions(-) I like it. Nice diffstat, too. There are some _nofail functions left, but none of them can use error_abort. If anything assigns to error_abort, we're probably screwed. No bright idea how to prevent that at compile-time.
On Tue, 03 Dec 2013 10:44:57 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > > > Following our discussion RE self asserting API calls, here is a spin of > > my proposal. This series obsoletes the need for _nofail variants for > > Error ** accepting APIs. Is also greately reduces the verbosity of calls > > sites that are currently asserting against errors. > > > > Patch 1 is the main event - addition of error_abort. The following > > patches then cleanup uses of _nofail and assert_no_error(). > > > > To give it a smoke test, I introduce a (critical) bug into QOM: > [...] > > 32 files changed, 100 insertions(+), 143 deletions(-) > > I like it. Nice diffstat, too. > > There are some _nofail functions left, but none of them can use > error_abort. > > If anything assigns to error_abort, we're probably screwed. No bright > idea how to prevent that at compile-time. Maybe Error * const error_abort;
Il 03/12/2013 10:44, Markus Armbruster ha scritto: > > There are some _nofail functions left, but none of them can use > error_abort. > > If anything assigns to error_abort, we're probably screwed. No bright > idea how to prevent that at compile-time. Isn't it always used by address? Paolo
On Tue, Dec 3, 2013 at 9:57 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 03/12/2013 10:44, Markus Armbruster ha scritto: >> >> There are some _nofail functions left, but none of them can use >> error_abort. >> >> If anything assigns to error_abort, we're probably screwed. No bright >> idea how to prevent that at compile-time. > > Isn't it always used by address? > Yes, but my implementation has reliance on it being null. That can be fixed with a bit a statement re-ordering. Regards, Peter > Paolo >
On 12/03/2013 02:44 AM, Markus Armbruster wrote: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> Following our discussion RE self asserting API calls, here is a spin of >> my proposal. This series obsoletes the need for _nofail variants for >> Error ** accepting APIs. Is also greately reduces the verbosity of calls >> sites that are currently asserting against errors. >> >> Patch 1 is the main event - addition of error_abort. The following >> patches then cleanup uses of _nofail and assert_no_error(). >> >> To give it a smoke test, I introduce a (critical) bug into QOM: > [...] >> 32 files changed, 100 insertions(+), 143 deletions(-) > > I like it. Nice diffstat, too. > > There are some _nofail functions left, but none of them can use > error_abort. > Also, is it worth adding asserts and/or compiler annotations to require that the Error **err argument of functions be non-NULL, to ensure that callers are always passing either a valid destination or one of the special addresses? But doing so would probably require adding a special address for error_ignore for callers that intend to discard an error in cases where the return type of the function lets them know to proceed with a fallback implementation (that is, cases where ignoring an error makes sense).
Eric Blake <eblake@redhat.com> writes: > On 12/03/2013 02:44 AM, Markus Armbruster wrote: >> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >> >>> Following our discussion RE self asserting API calls, here is a spin of >>> my proposal. This series obsoletes the need for _nofail variants for >>> Error ** accepting APIs. Is also greately reduces the verbosity of calls >>> sites that are currently asserting against errors. >>> >>> Patch 1 is the main event - addition of error_abort. The following >>> patches then cleanup uses of _nofail and assert_no_error(). >>> >>> To give it a smoke test, I introduce a (critical) bug into QOM: >> [...] >>> 32 files changed, 100 insertions(+), 143 deletions(-) >> >> I like it. Nice diffstat, too. >> >> There are some _nofail functions left, but none of them can use >> error_abort. >> > > Also, is it worth adding asserts and/or compiler annotations to require > that the Error **err argument of functions be non-NULL, to ensure that > callers are always passing either a valid destination or one of the > special addresses? But doing so would probably require adding a special > address for error_ignore for callers that intend to discard an error in > cases where the return type of the function lets them know to proceed > with a fallback implementation (that is, cases where ignoring an error > makes sense). Right now, we use NULL as "ignore errors" argument. NULL gives us a chance to express "caller must not ignore errors" via some non-null annotation that gets fed to a static analyzer. I doubt that would be possible with a special error_ignore object. Anyway, this series is about "abort on error". Let's keep "ignore errors" issues separate.
On Tue, 03 Dec 2013 14:53:06 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 12/03/2013 02:44 AM, Markus Armbruster wrote: > >> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> > >>> Following our discussion RE self asserting API calls, here is a spin of > >>> my proposal. This series obsoletes the need for _nofail variants for > >>> Error ** accepting APIs. Is also greately reduces the verbosity of calls > >>> sites that are currently asserting against errors. > >>> > >>> Patch 1 is the main event - addition of error_abort. The following > >>> patches then cleanup uses of _nofail and assert_no_error(). > >>> > >>> To give it a smoke test, I introduce a (critical) bug into QOM: > >> [...] > >>> 32 files changed, 100 insertions(+), 143 deletions(-) > >> > >> I like it. Nice diffstat, too. > >> > >> There are some _nofail functions left, but none of them can use > >> error_abort. > >> > > > > Also, is it worth adding asserts and/or compiler annotations to require > > that the Error **err argument of functions be non-NULL, to ensure that > > callers are always passing either a valid destination or one of the > > special addresses? But doing so would probably require adding a special > > address for error_ignore for callers that intend to discard an error in > > cases where the return type of the function lets them know to proceed > > with a fallback implementation (that is, cases where ignoring an error > > makes sense). > > Right now, we use NULL as "ignore errors" argument. > > NULL gives us a chance to express "caller must not ignore errors" via > some non-null annotation that gets fed to a static analyzer. > > I doubt that would be possible with a special error_ignore object. > > Anyway, this series is about "abort on error". Let's keep "ignore > errors" issues separate. I'm sorry for hijacking thread, but that actually an issue that started an original discussion. Where void returning QOM API functions are used with NULL, without any chance to detect that error happened. So abusing NULL errp in this functions might lead to hard to find runtime errors. I think Eric's suggestion was to enforce passing non NULL errp and let caller to deal with error gracefully so that above mentioned misuse was impossible. Why is ignoring errors from "void foo(...)" like API considered acceptable?
On 12/03/2013 01:33 PM, Igor Mammedov wrote: >>> Also, is it worth adding asserts and/or compiler annotations to require >>> that the Error **err argument of functions be non-NULL, to ensure that >>> callers are always passing either a valid destination or one of the >>> special addresses? But doing so would probably require adding a special >>> address for error_ignore for callers that intend to discard an error in >>> cases where the return type of the function lets them know to proceed >>> with a fallback implementation (that is, cases where ignoring an error >>> makes sense). >> >> Right now, we use NULL as "ignore errors" argument. >> >> NULL gives us a chance to express "caller must not ignore errors" via >> some non-null annotation that gets fed to a static analyzer. >> >> I doubt that would be possible with a special error_ignore object. >> >> Anyway, this series is about "abort on error". Let's keep "ignore >> errors" issues separate. > I'm sorry for hijacking thread, but that actually an issue that started an > original discussion. > Where void returning QOM API functions are used with NULL, without any chance > to detect that error happened. So abusing NULL errp in this functions > might lead to hard to find runtime errors. > I think Eric's suggestion was to enforce passing non NULL errp and let caller > to deal with error gracefully so that above mentioned misuse was impossible. > Why is ignoring errors from "void foo(...)" like API considered acceptable? Okay, so it sounds like consensus is that using NULL as the means to ignore errors is okay when there is an alternative way to detect error, but that for any function that returns void, adding an assert(errp) would be appropriate because the caller cannot safely ignore the failure. It's not worth inventing an error_ignore special address, but for functions that have no way to report errors except via errp, then it IS worth enforcing that the caller is either prepared to handle the error or has passed &error_abort (or any other special addresses we add later).
Eric Blake <eblake@redhat.com> writes: > On 12/03/2013 01:33 PM, Igor Mammedov wrote: > >>>> Also, is it worth adding asserts and/or compiler annotations to require >>>> that the Error **err argument of functions be non-NULL, to ensure that >>>> callers are always passing either a valid destination or one of the >>>> special addresses? But doing so would probably require adding a special >>>> address for error_ignore for callers that intend to discard an error in >>>> cases where the return type of the function lets them know to proceed >>>> with a fallback implementation (that is, cases where ignoring an error >>>> makes sense). >>> >>> Right now, we use NULL as "ignore errors" argument. >>> >>> NULL gives us a chance to express "caller must not ignore errors" via >>> some non-null annotation that gets fed to a static analyzer. >>> >>> I doubt that would be possible with a special error_ignore object. >>> >>> Anyway, this series is about "abort on error". Let's keep "ignore >>> errors" issues separate. >> I'm sorry for hijacking thread, but that actually an issue that started an >> original discussion. >> Where void returning QOM API functions are used with NULL, without any chance >> to detect that error happened. So abusing NULL errp in this functions >> might lead to hard to find runtime errors. >> I think Eric's suggestion was to enforce passing non NULL errp and let caller >> to deal with error gracefully so that above mentioned misuse was impossible. >> Why is ignoring errors from "void foo(...)" like API considered acceptable? > > Okay, so it sounds like consensus is that using NULL as the means to > ignore errors is okay when there is an alternative way to detect error, > but that for any function that returns void, adding an assert(errp) > would be appropriate because the caller cannot safely ignore the > failure. It's not worth inventing an error_ignore special address, but > for functions that have no way to report errors except via errp, then it > IS worth enforcing that the caller is either prepared to handle the > error or has passed &error_abort (or any other special addresses we add > later). No objection to asserting that the caller passed an error object when the error object is the only way to signal failure. You can't force your callers to check for failure, but the assertion could help prevent accidental misuse. Assertions fire at run-time, though. Asserting "argument not null" first thing in the function should enable a sufficiently smart whole-program static checker to flag null arguments. But having such a static check right at compile-time would be much better. Could attribute nonnull do it? If yes, do we still need the assertion?
On 12/04/2013 02:11 AM, Markus Armbruster wrote: > No objection to asserting that the caller passed an error object when > the error object is the only way to signal failure. You can't force > your callers to check for failure, but the assertion could help prevent > accidental misuse. > > Assertions fire at run-time, though. Unfortunately true. > > Asserting "argument not null" first thing in the function should enable > a sufficiently smart whole-program static checker to flag null > arguments. Coverity is such a checker; I think clang can as well. > > But having such a static check right at compile-time would be much > better. Could attribute nonnull do it? If yes, do we still need the > assertion? gcc's implementation of attribute nonnull is complete trash. And the gcc developers know it. The attribute is still useful for Coverity, but at least in libvirt, we have taken to using the attribute ONLY when compiling under a static checker and omitting it under gcc because gcc's implementation of the attribute is so horribly botched. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 So even with attribute nonnull, you still need the assertion.
Il 03/12/2013 21:33, Igor Mammedov ha scritto: > I'm sorry for hijacking thread, but that actually an issue that started an > original discussion. > Where void returning QOM API functions are used with NULL, without any chance > to detect that error happened. So abusing NULL errp in this functions > might lead to hard to find runtime errors. > I think Eric's suggestion was to enforce passing non NULL errp and let caller > to deal with error gracefully so that above mentioned misuse was impossible. > Why is ignoring errors from "void foo(...)" like API considered acceptable? See http://permalink.gmane.org/gmane.comp.emulators.qemu/243779 > * Peter's alternative > + self-documenting > + consistent > + predictable I'll add another small advantage which is fewer SLOC. > * make Error* mandatory for all void functions > + consistent > + almost predictable (because in C you can ignore return values) > - not necessarily does the right thing (e.g. cleanup functions) > - requires manual effort to abide to the policy Better wording of the last: a missing &error_abort is easier to spot than a missing assert_no_error(errp). Paolo
On Thu, 05 Dec 2013 11:37:27 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 03/12/2013 21:33, Igor Mammedov ha scritto: > > I'm sorry for hijacking thread, but that actually an issue that started an > > original discussion. > > Where void returning QOM API functions are used with NULL, without any chance > > to detect that error happened. So abusing NULL errp in this functions > > might lead to hard to find runtime errors. > > I think Eric's suggestion was to enforce passing non NULL errp and let caller > > to deal with error gracefully so that above mentioned misuse was impossible. > > Why is ignoring errors from "void foo(...)" like API considered acceptable? > > See http://permalink.gmane.org/gmane.comp.emulators.qemu/243779 > > > * Peter's alternative > > + self-documenting > > + consistent > > + predictable > > I'll add another small advantage which is fewer SLOC. There is not argument against Peter's approach at all, question is what do we do with NULL errp in void API functions? > > > * make Error* mandatory for all void functions one more advantage: + not need to pepper every property setter/getter with local_error + error_propagate(), i.e. reduced code duplication. > > + consistent > > + almost predictable (because in C you can ignore return values) there is no return values from void functions. > > - not necessarily does the right thing (e.g. cleanup functions) we can pass &error_abort instead of NULL there if we don't care. If there will be error it would mean something went horribly wrong and perhaps code should care if error happens there. for special cases we could invent &ignore_error if there will be real need for it. > > - requires manual effort to abide to the policy with assert inside API there is no manual effort. But as Marcus noted these errors will be only runtime detectable :( > > Better wording of the last: a missing &error_abort is easier to spot > than a missing assert_no_error(errp). > > Paolo
Il 05/12/2013 16:32, Igor Mammedov ha scritto: >>> > > * make Error* mandatory for all void functions > one more advantage: > + not need to pepper every property setter/getter with local_error + error_propagate(), > i.e. reduced code duplication. Note that for something like tail calls there is no need for error_propagate. See get_enum/set_enum (and a lot more examples) in hw/core/qdev-properties.c. With your proposal, this: visit_type_bool(v, &value, name, &local_err); if (local_err) { error_propagate(errp, local_err); return; } bit_prop_set(dev, prop, value); could indeed become visit_type_bool(v, &value, name, errp); bit_prop_set(dev, prop, value); because both caller and callee return void. This is because both the caller (prop_set_bit) and callee (visit_type_bool) are void. But here come the next point: >>> > > + consistent >>> > > + almost predictable (because in C you can ignore return values) > there is no return values from void functions. You can ignore return values from int-returning functions. So if I see func(..., NULL); func(..., errp); it's not clear if it aborts on error, or just eats the error. > > - requires manual effort to abide to the policy > with assert inside API there is no manual effort. But as Marcus > noted these errors will be only runtime detectable :( I referred to manual effort of adding assertions in leaf functions. Just one missing assertion can be very problematic if non-leafs start relying on this behavior. I think Error is hard to misuse if you don't mind being verbose. This proposal would cut the verbosity (not sure how much, but it definitely could) but it would be easier to misuse. In terms of Rusty's API design manifesto (http://sweng.the-davies.net/Home/rustys-api-design-manifesto) your proposal would move the API from 4 to 3 or 2: 4. Follow common convention and you'll get it right. 3. Read the documentation and you'll get it right. 2. Read the implementation and you'll get it right. Paolo
--- a/qom/object.c +++ b/qom/object.c @@ -797,7 +797,7 @@ void object_property_set(Object *obj, Visitor *v, const char *name, return; } - if (!prop->set) { + if (prop->set) { error_set(errp, QERR_PERMISSION_DENIED); } else {
Following our discussion RE self asserting API calls, here is a spin of my proposal. This series obsoletes the need for _nofail variants for Error ** accepting APIs. Is also greately reduces the verbosity of calls sites that are currently asserting against errors. Patch 1 is the main event - addition of error_abort. The following patches then cleanup uses of _nofail and assert_no_error(). To give it a smoke test, I introduce a (critical) bug into QOM: prop->set(obj, v, prop->opaque, name, errp); And run QEMU: $ gdb --args ./microblazeel-softmmu/qemu-system-microblazeel -M petalogix-ml605 -nographic Without application of this series, the bug manifests as: qemu-system-microblazeel: Insufficient permission to perform this operation Program received signal SIGABRT, Aborted. (gdb) bt 0 0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 1 0x00007ffff55fab8b in __GI_abort () at abort.c:91 2 0x00005555557073da in assert_no_error (err=<optimized out>) at qobject/qerror.c:128 3 0x0000555555615159 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x7fffffffe3c0) at hw/core/qdev.c:666 4 0x0000555555615355 in device_initfn (obj=<optimized out>) at hw/core/qdev.c:744 With this series, we now get a the full backtrace into the QOM API when the assertion occurs (note stack frames 2-4 giving visibility into the broken QOM API): qemu-system-microblazeel: Insufficient permission to perform this operation Program received signal SIGABRT, Aborted. (gdb) bt 0 0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 1 0x00007ffff55fab8b in __GI_abort () at abort.c:91 2 0x000055555570c5a4 in error_set (errp=0x555555e1b5f0, err_class=ERROR_CLASS_GENERIC_ERROR, fmt=0x55555571f5c0 "Insufficient permission to perform this operation") at util/error.c:47 3 0x00005555556778e5 in object_property_set_qobject (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/qom-qobject.c:24 4 0x000055555567674d in object_property_set_int (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/object.c:898 5 0x0000555555615192 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x555555e1b5f0) at hw/core/qdev.c:664 6 0x000055555561534f in device_initfn (obj=<optimized out>) at hw/core/qdev.c:741 Peter Crosthwaite (5): error: Add error_abort hw: Remove assert_no_error usages target-i386: Remove assert_no_error usage qemu-option: Remove qemu_opts_create_nofail qerror: Remove assert_no_error() block/blkdebug.c | 2 +- block/blkverify.c | 2 +- block/curl.c | 2 +- block/gluster.c | 2 +- block/iscsi.c | 2 +- block/nbd.c | 2 +- block/qcow2.c | 2 +- block/raw-posix.c | 2 +- block/raw-win32.c | 4 ++-- block/rbd.c | 2 +- block/sheepdog.c | 2 +- block/vvfat.c | 2 +- blockdev.c | 6 ++++-- hw/core/qdev-properties-system.c | 8 ++------ hw/core/qdev-properties.c | 40 ++++++++++------------------------------ hw/core/qdev.c | 28 +++++++--------------------- hw/dma/xilinx_axidma.c | 13 ++++--------- hw/net/xilinx_axienet.c | 13 ++++--------- hw/watchdog/watchdog.c | 3 ++- include/hw/xilinx.h | 14 ++++---------- include/qapi/error.h | 6 ++++++ include/qapi/qmp/qerror.h | 1 - include/qemu/option.h | 1 - qdev-monitor.c | 2 +- qemu-img.c | 2 +- qobject/qerror.c | 8 -------- target-i386/cpu.c | 4 +--- util/error.c | 22 +++++++++++++++++++++- util/qemu-config.c | 2 +- util/qemu-option.c | 9 --------- util/qemu-sockets.c | 18 +++++++++--------- vl.c | 17 ++++++++++------- 32 files changed, 100 insertions(+), 143 deletions(-)