Re: [Qemu-devel] [RFC PATCH v1 00/25] error: Automatic error concatenation and prefixing

[Eric Blake]

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 <address@hidden>
> ---
>  __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