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

[Peter Crosthwaite]

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 <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
-- 
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