From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60474) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaGyB-0004oK-SC for qemu-devel@nongnu.org; Fri, 11 Sep 2015 01:33:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZaGy6-0001nt-PB for qemu-devel@nongnu.org; Fri, 11 Sep 2015 01:33:55 -0400 Received: from mail-pa0-x22c.google.com ([2607:f8b0:400e:c03::22c]:35380) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaGy6-0001ng-Bp for qemu-devel@nongnu.org; Fri, 11 Sep 2015 01:33:50 -0400 Received: by pacfv12 with SMTP id fv12so65799861pac.2 for ; Thu, 10 Sep 2015 22:33:49 -0700 (PDT) From: Peter Crosthwaite Date: Thu, 10 Sep 2015 22:33:10 -0700 Message-Id: Subject: [Qemu-devel] [RFC PATCH v1 00/25] error: Automatic error concatenation and prefixing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, armbru@redhat.com Hi Markus and all, This patch series adds support for automatically concatenating multiple errors to the one Error *. I'll start with what I am actually trying to do, which is get rid of the boilerplate: some_api_call(... , &local_err); if (local_err) { error_propagate(errp, local_err); return; } some_similar_api_call(... , &local_err); if (local_err) { error_propagate(errp, local_err); return; } some_similar_api_call(... , &local_err); if (local_err) { error_propagate(errp, local_err); return; } ... It is usually 1 LOC for the API and 4 LOC for the error handling boiler plate making our code hard reading. This is particularly bad in hw/arm where we have a good number of fully QOMified SoCs and machine models, each of which need these checks on recursive realize calls and friends. The removed LOC just in ARM pays for the extra core code needed to implement this. And the number of ARM machines is only going to grow. So the plan is: 1: Allow an Error * to contain more that one actual error from API calls. 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); 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. 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. Regards, Peter --END--- Signed-off-by: Peter Crosthwaite --- __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 -- 1.9.1 Peter Crosthwaite (25): exec: convert error_report to error_report_err s390x: virtio-ccw: Remove un-needed if guard error: Factor out common error setter logic error: Add support for multiple errors error: Add error prefix API error: Add error_printf_fn() sysbus: mmio_map+mmio_get_region: ignore range OOB errors memory: nop APIs when they have NULL arguments qdev: gpio: Ignore unconnectable GPIOs arm: xlnx-zynqmp: Update error API usages arm: fsl-imx*: Update error API usages arm: netduino: Update error API usages arm: allwinner: Update error API usages arm: digic: Update error API usages cpu: arm: Remove un-needed error checking ppc: Update error API usages i386: pc: Update error API usages monitor: update error API usages qdev: Update error API usages block: Update error API usages tests: Update error API usages usb: bus: Update error API usages scsi: Update error API usages migration: savevm: Update error API usages core: Update error API usages arch_init.c | 5 +- block.c | 5 +- block/qcow2.c | 5 +- block/qed.c | 5 +- block/sheepdog.c | 8 +-- blockdev.c | 10 +-- exec.c | 2 +- hmp.c | 33 ++++----- hw/arm/allwinner-a10.c | 18 +---- hw/arm/cubieboard.c | 28 +++----- hw/arm/digic.c | 19 +---- hw/arm/digic_boards.c | 4 +- hw/arm/fsl-imx25.c | 46 +----------- hw/arm/fsl-imx31.c | 42 +---------- hw/arm/netduino2.c | 2 +- hw/arm/stm32f205_soc.c | 14 +--- hw/arm/xilinx_zynq.c | 14 +--- hw/arm/xlnx-ep108.c | 2 +- hw/arm/xlnx-zynqmp.c | 29 +------- hw/block/dataplane/virtio-blk.c | 5 +- hw/core/qdev-properties.c | 7 +- hw/core/qdev.c | 15 ++-- hw/core/sysbus.c | 11 ++- hw/cpu/a15mpcore.c | 6 +- hw/cpu/a9mpcore.c | 22 +----- hw/cpu/arm11mpcore.c | 18 +---- hw/cpu/realview_mpcore.c | 9 +-- hw/i386/pc.c | 5 +- hw/ppc/e500.c | 4 +- hw/ppc/spapr.c | 4 +- hw/ppc/spapr_drc.c | 6 +- hw/s390x/virtio-ccw.c | 28 ++------ hw/scsi/vhost-scsi.c | 5 +- hw/usb/bus.c | 10 ++- include/monitor/monitor.h | 2 +- include/qapi/error.h | 13 ++++ memory.c | 9 +++ migration/savevm.c | 22 +++--- monitor.c | 11 ++- qapi/common.json | 5 +- qemu-img.c | 38 +++++----- stubs/mon-printf.c | 2 +- tests/test-aio.c | 5 +- tests/test-thread-pool.c | 5 +- tests/test-throttle.c | 9 ++- ui/vnc.c | 5 +- util/error.c | 154 ++++++++++++++++++++++++++-------------- vl.c | 7 +- 48 files changed, 281 insertions(+), 452 deletions(-) -- 1.9.1