On 09/10/2015 11:33 PM, Peter Crosthwaite wrote: > Hi Markus and all, > > This patch series adds support for automatically concatenating multiple > errors to the one Error *.
Sounds interesting! > So the plan is: > > 1: Allow an Error * to contain more that one actual error from API > calls. Is this for all errors, or do you have to make a special call to make it obvious that a particular error can be concatenated to while the default is still to report programming error if a second error is attempted atop a regular error that hasn't requested concatenation? > 2: Refactor key APIs (some_similar_api_call() in the above example) > to not fatal when previous errors have occured in dependencies. > > Point 1 kind of got big on me. Patch 4 is the main event, listifying > errors. The follow up issue however, is it tricky to get a sane > definition of error_get_pretty for a multi-error. So instead the > strategy is to remove error_get_pretty() and replace with some error > API helper with well defined behaviour for multi-error. The two leading > uses of error get pretty are prefixing an error, and reporting an error > via a custom printf handler. So two new APIs are added for that (P5-6). > There aren't many error_get_pretty's left after that, and they > shouldn't be in the path of any multi-errors. > > I think the error_prefix is valuable it its own right, as it now means > the code for report or propagating a prefixed error is now consistent > with the non-prefixed variants. > > That is, we used to have: > > /* If we are prefixing we have to do it this way: */ > error_setg(errp, "some prefix %s", error_get_pretty(local_err)); > error_free(local_err); > > vs: > > /* but if not prefixing it is like this: */ > error_propagate(errp, local_err); > > Now with this patch series the two are much more recognisable as the same > with: > > /* This code is almost the same as the above non-prefixed propagation */ > error_prefix(local_err, "some prefix"): > error_propagate(errp, local_err); Seems nice in its own right. > > Point 2 is less about error API and more about machine generation. > Sysbus, Memory and Qdev all have APIs that depend on successful device- > init and realize calls for argument devices. As we are trying to remove > the error detection for those argument devs, those APIs need to tweaked > to handle realize failure. This actually wasn't as bad as I thought it > would be. See patches (7-9). > > All patches after that walk the various major subsystems converting > error APIs to this new scheme and deleting now-unneeded boiler plate. > ARM is first (P10-15) seeing good clean up of propagate handers. > > So the net result for these ARM machines, is error behaviour that is > something like a compiler. If any one thing fails, then machine-init > (compilation) fails. But an early fail does not stop machine-init > (compilation), instead it proceeds to the end collecting subsequent > errors as it goes. Sometimes that causes more problems (ignoring an error and proceeding on can cause confusing followup errors), but usually it manages to work out. > > Some other interesting food for thought is the qemu_fdt APIs which I > have been wanting to convert to error API but the local_err propagation > is going to be brutal in heavy users like e500.c. This would solve that > as fdt API could be easily made multi-error safe and clients like e500 > can just collect multi-errors and single-fail at the end. > > Long term, we can use this to catch cases of multiple genuine machine > init errors in the one boot but that is a secondary goal to simply > cutting down on code boilerplate. The best feature of this series is > the diffstat. > > Patches 1-3 are cleanup that can be taken independent of the series. > > I think P3 may be obsolete from a recent merge, but i'll wait > for architectural feedback before rebasing. Yeah, both Markus and I have been touching error.c lately, so a rebase will probably be needed. > > Regards, > Peter > --END--- > > Signed-off-by: Peter Crosthwaite <crosthwaite.pe...@gmail.com> > --- > __HAS_COVER__ | 0 > 1 file changed, 0 insertions(+), 0 deletions(-) > create mode 100644 __HAS_COVER__ > > diff --git a/__HAS_COVER__ b/__HAS_COVER__ > new file mode 100644 > index 0000000..e69de29 > Huh - whatever you are using to version your cover letter made the real diffstat be part of the signature rather than the main body of the email. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
