[RFC v2 0/9] error: auto propagated local_err

[RFC v2 0/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a proposal of auto propagation for local_err, to not call
error_propagate on every exit point, when we deal with local_err.

It also fixes two issues:
1. Fix issue with error_fatal & error_append_hint: user can't see these
hints, because exit() happens in error_setg earlier than hint is
appended. [Reported by Greg Kurz]

2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself don't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
[Reported by Kevin Wolf]

It's still an RFC, due to the following reasons:

1. I'm new to coccinella, so I failed to do the following pattern:

 <...
- goto out;
+ return;
 ...>
- out:
- error_propagate(errp, local_err)

So, here is compilation fix 08.. Who can help with it? If nobody, 08 is
to be merged to 07 by hand.

2. Question about using new macro in empty stub functions - see 09

3. What to do with huge auto-generated commit 07? Should I split it
per-maintainer or per-subsystem, or leave it as-is?

4. Also, checkpatch has some complains about 07 patch:
  - using tabs.. (hmm exactly stubs functions..)
  - empty ifs
  Again, I don't see any ways to fix it other that by hand and merge to
  07..

==================

Also, if we decide, that this all is too huge, here is plan B:

1. apply 01
2. fix only functions that don't use local_err and use
error_append_hint, by just invocation of new macro at function start -
it will substitute Greg's series with no pain.
3[optional]. Do full update for some subsystems, for example, only for
block* and nbd*

Vladimir Sementsov-Ogievskiy (9):
  error: auto propagated local_err
  qapi/error: add (Error **errp) cleaning APIs
  errp: rename errp to errp_in where it is IN-argument
  hw/core/loader-fit: fix freeing errp in fit_load_fdt
  net/net: fix local variable shadowing in net_client_init
  scripts: add coccinelle script to use auto propagated errp
  Use auto-propagated errp
  fix-compilation: empty goto
  fix-compilation: includes

 include/hw/pci-host/spapr.h                   |   2 +
 include/monitor/hmp.h                         |   2 +-
 include/qapi/error.h                          |  61 ++++-
 target/ppc/kvm_ppc.h                          |   3 +
 target/s390x/cpu_models.h                     |   3 +
 ui/vnc.h                                      |   2 +-
 audio/audio.c                                 |  12 +-
 authz/pamacct.c                               |   1 +
 backends/cryptodev-vhost-user.c               |   9 +-
 backends/cryptodev.c                          |  24 +-
 backends/hostmem-file.c                       |  19 +-
 backends/hostmem-memfd.c                      |  17 +-
 backends/hostmem.c                            |  38 ++-
 backends/rng.c                                |   6 +-
 block.c                                       | 208 +++++++----------
 block/blkdebug.c                              |  33 +--
 block/blklogwrites.c                          |  21 +-
 block/blkreplay.c                             |   6 +-
 block/blkverify.c                             |  16 +-
 block/block-backend.c                         |  17 +-
 block/commit.c                                |   6 +-
 block/crypto.c                                |  12 +-
 block/curl.c                                  |   6 +-
 block/file-posix.c                            |  71 +++---
 block/file-win32.c                            |  27 +--
 block/gluster.c                               |  64 +++--
 block/io.c                                    |  11 +-
 block/iscsi.c                                 |  33 +--
 block/mirror.c                                |  17 +-
 block/nbd.c                                   |  44 ++--
 block/nfs.c                                   |   6 +-
 block/nvme.c                                  |  17 +-
 block/parallels.c                             |  28 +--
 block/qapi.c                                  |  23 +-
 block/qcow.c                                  |  15 +-
 block/qcow2-bitmap.c                          |   6 +-
 block/qcow2.c                                 |  90 +++----
 block/qed.c                                   |  16 +-
 block/quorum.c                                |  22 +-
 block/raw-format.c                            |   6 +-
 block/rbd.c                                   |  25 +-
 block/replication.c                           |  36 ++-
 block/sheepdog.c                              |  66 +++---
 block/snapshot.c                              |  14 +-
 block/ssh.c                                   |  11 +-
 block/throttle-groups.c                       |  22 +-
 block/throttle.c                              |   6 +-
 block/vdi.c                                   |  12 +-
 block/vhdx.c                                  |  20 +-
 block/vmdk.c                                  |  36 +--
 block/vpc.c                                   |  25 +-
 block/vvfat.c                                 |  11 +-
 block/vxhs.c                                  |  22 +-
 blockdev.c                                    | 220 +++++++-----------
 blockjob.c                                    |   7 +-
 bootdevice.c                                  |  29 +--
 chardev/char-socket.c                         |   6 +-
 chardev/char.c                                |  18 +-
 crypto/block-luks.c                           |  31 +--
 crypto/secret.c                               |  16 +-
 crypto/tlssession.c                           |   6 +-
 dump/dump.c                                   | 142 +++++------
 dump/win_dump.c                               |  27 +--
 exec.c                                        |  12 +-
 hw/9pfs/9p-local.c                            |   7 +-
 hw/acpi/core.c                                |  17 +-
 hw/acpi/ich9.c                                |  27 +--
 hw/acpi/memory_hotplug.c                      |   6 +-
 hw/arm/allwinner-a10.c                        |  26 +--
 hw/arm/armv7m.c                               |  51 ++--
 hw/arm/bcm2835_peripherals.c                  |  84 +++----
 hw/arm/bcm2836.c                              |  40 ++--
 hw/arm/digic.c                                |  21 +-
 hw/arm/fsl-imx25.c                            |  61 ++---
 hw/arm/fsl-imx31.c                            |  56 ++---
 hw/arm/fsl-imx6.c                             |  80 +++----
 hw/arm/integratorcp.c                         |   6 +-
 hw/arm/msf2-soc.c                             |  21 +-
 hw/arm/nrf51_soc.c                            |  46 ++--
 hw/arm/smmu-common.c                          |   6 +-
 hw/arm/smmuv3.c                               |   6 +-
 hw/arm/stm32f205_soc.c                        |  38 ++-
 hw/arm/tosa.c                                 |   1 +
 hw/arm/xlnx-versal-virt.c                     |   6 +-
 hw/arm/xlnx-zynqmp.c                          |  84 +++----
 hw/audio/intel-hda.c                          |  12 +-
 hw/block/dataplane/xen-block.c                |  16 +-
 hw/block/fdc.c                                |  17 +-
 hw/block/onenand.c                            |   6 +-
 hw/block/pflash_cfi01.c                       |   6 +-
 hw/block/pflash_cfi02.c                       |   6 +-
 hw/block/vhost-user-blk.c                     |   5 +-
 hw/block/virtio-blk.c                         |   6 +-
 hw/block/xen-block.c                          | 114 ++++-----
 hw/char/debugcon.c                            |   6 +-
 hw/char/serial-pci-multi.c                    |   6 +-
 hw/char/serial-pci.c                          |   6 +-
 hw/char/virtio-serial-bus.c                   |   6 +-
 hw/core/bus.c                                 |  14 +-
 hw/core/loader-fit.c                          |   2 +-
 hw/core/machine.c                             |  18 +-
 hw/core/numa.c                                |  48 ++--
 hw/core/qdev-properties-system.c              |  24 +-
 hw/core/qdev-properties.c                     |  78 +++----
 hw/core/qdev.c                                |  36 ++-
 hw/core/sysbus.c                              |   1 +
 hw/cpu/a15mpcore.c                            |   6 +-
 hw/cpu/a9mpcore.c                             |  26 +--
 hw/cpu/arm11mpcore.c                          |  21 +-
 hw/cpu/core.c                                 |  12 +-
 hw/cpu/realview_mpcore.c                      |  11 +-
 hw/display/bcm2835_fb.c                       |   5 +-
 hw/display/qxl.c                              |   6 +-
 hw/display/virtio-gpu-base.c                  |   6 +-
 hw/display/virtio-gpu-pci.c                   |   6 +-
 hw/display/virtio-vga.c                       |   6 +-
 hw/dma/bcm2835_dma.c                          |   5 +-
 hw/dma/xilinx_axidma.c                        |  21 +-
 hw/gpio/aspeed_gpio.c                         |   6 +-
 hw/gpio/bcm2835_gpio.c                        |   9 +-
 hw/i386/kvm/apic.c                            |   2 +
 hw/i386/pc.c                                  | 109 ++++-----
 hw/ide/qdev.c                                 |  15 +-
 hw/input/virtio-input.c                       |  12 +-
 hw/intc/apic_common.c                         |   6 +-
 hw/intc/arm_gic.c                             |   6 +-
 hw/intc/arm_gic_kvm.c                         |  11 +-
 hw/intc/arm_gicv3.c                           |  11 +-
 hw/intc/arm_gicv3_its_kvm.c                   |   6 +-
 hw/intc/arm_gicv3_kvm.c                       |  16 +-
 hw/intc/armv7m_nvic.c                         |  11 +-
 hw/intc/nios2_iic.c                           |   5 +-
 hw/intc/pnv_xive.c                            |  14 +-
 hw/intc/realview_gic.c                        |   6 +-
 hw/intc/s390_flic_kvm.c                       |  10 +-
 hw/intc/spapr_xive.c                          |  11 +-
 hw/intc/spapr_xive_kvm.c                      |  49 ++--
 hw/intc/xics.c                                |  31 +--
 hw/intc/xics_kvm.c                            |  28 +--
 hw/intc/xics_pnv.c                            |   6 +-
 hw/intc/xive.c                                |  23 +-
 hw/ipack/ipack.c                              |   4 +-
 hw/isa/pc87312.c                              |   6 +-
 hw/mem/memory-device.c                        |  19 +-
 hw/mem/nvdimm.c                               |  23 +-
 hw/mem/pc-dimm.c                              |  21 +-
 hw/microblaze/xlnx-zynqmp-pmu.c               |  11 +-
 hw/mips/cps.c                                 |  45 ++--
 hw/misc/arm11scu.c                            |   2 +
 hw/misc/bcm2835_mbox.c                        |   5 +-
 hw/misc/bcm2835_property.c                    |   9 +-
 hw/misc/ivshmem.c                             |  33 +--
 hw/misc/macio/macio.c                         |  65 ++----
 hw/misc/mps2-scc.c                            |   2 +
 hw/misc/tmp105.c                              |   6 +-
 hw/misc/tmp421.c                              |   6 +-
 hw/net/dp8393x.c                              |   6 +-
 hw/net/eepro100.c                             |   6 +-
 hw/net/ne2000-isa.c                           |  16 +-
 hw/net/xilinx_axienet.c                       |  21 +-
 hw/nvram/fw_cfg.c                             |  12 +-
 hw/nvram/nrf51_nvm.c                          |   6 +-
 hw/pci-bridge/dec.c                           |   2 +
 hw/pci-bridge/gen_pcie_root_port.c            |   6 +-
 hw/pci-bridge/pci_bridge_dev.c                |  12 +-
 hw/pci-bridge/pci_expander_bridge.c           |   6 +-
 hw/pci-bridge/pcie_pci_bridge.c               |   7 +-
 hw/pci-host/piix.c                            |   6 +-
 hw/pci/pci.c                                  |  17 +-
 hw/pci/pcie.c                                 |   6 +-
 hw/pci/shpc.c                                 |  12 +-
 hw/ppc/e500.c                                 |   6 +-
 hw/ppc/pnv.c                                  |  92 +++-----
 hw/ppc/pnv_core.c                             |  21 +-
 hw/ppc/pnv_lpc.c                              |  22 +-
 hw/ppc/pnv_occ.c                              |   4 +-
 hw/ppc/pnv_psi.c                              |  21 +-
 hw/ppc/spapr.c                                | 124 ++++------
 hw/ppc/spapr_caps.c                           |  50 ++--
 hw/ppc/spapr_cpu_core.c                       |  33 ++-
 hw/ppc/spapr_drc.c                            |  44 ++--
 hw/ppc/spapr_irq.c                            |  95 +++-----
 hw/ppc/spapr_pci.c                            |  88 +++----
 hw/ppc/spapr_vio.c                            |  11 +-
 hw/riscv/riscv_hart.c                         |   7 +-
 hw/riscv/sifive_e.c                           |   6 +-
 hw/riscv/sifive_u.c                           |  10 +-
 hw/s390x/3270-ccw.c                           |  12 +-
 hw/s390x/css-bridge.c                         |   6 +-
 hw/s390x/css.c                                |   6 +-
 hw/s390x/ipl.c                                |  23 +-
 hw/s390x/s390-ccw.c                           |  17 +-
 hw/s390x/s390-pci-bus.c                       |  34 ++-
 hw/s390x/s390-skeys.c                         |   6 +-
 hw/s390x/s390-virtio-ccw.c                    |  10 +-
 hw/s390x/sclp.c                               |  14 +-
 hw/s390x/tod-kvm.c                            |  13 +-
 hw/s390x/virtio-ccw-crypto.c                  |   6 +-
 hw/s390x/virtio-ccw-rng.c                     |   6 +-
 hw/s390x/virtio-ccw.c                         |  12 +-
 hw/scsi/esp-pci.c                             |   6 +-
 hw/scsi/megasas.c                             |  10 +-
 hw/scsi/mptsas.c                              |  12 +-
 hw/scsi/scsi-bus.c                            |  22 +-
 hw/scsi/scsi-disk.c                           |   6 +-
 hw/scsi/vhost-scsi.c                          |  11 +-
 hw/scsi/vhost-user-scsi.c                     |   6 +-
 hw/scsi/virtio-scsi.c                         |   6 +-
 hw/sd/milkymist-memcard.c                     |  10 +-
 hw/sd/sdhci-pci.c                             |   6 +-
 hw/sd/sdhci.c                                 |  20 +-
 hw/sd/ssi-sd.c                                |  13 +-
 hw/smbios/smbios.c                            |  41 ++--
 hw/sparc/sun4m.c                              |  18 +-
 hw/sparc64/sun4u.c                            |   6 +-
 hw/timer/aspeed_timer.c                       |   6 +-
 hw/tpm/tpm_util.c                             |   6 +-
 hw/usb/bus.c                                  |  33 +--
 hw/usb/dev-serial.c                           |   6 +-
 hw/usb/dev-smartcard-reader.c                 |  12 +-
 hw/usb/dev-storage.c                          |  16 +-
 hw/usb/hcd-ohci-pci.c                         |   6 +-
 hw/usb/hcd-ohci.c                             |  12 +-
 hw/usb/hcd-uhci.c                             |   6 +-
 hw/usb/hcd-xhci.c                             |  12 +-
 hw/vfio/ap.c                                  |   9 +-
 hw/vfio/ccw.c                                 |  23 +-
 hw/vfio/pci-quirks.c                          |   6 +-
 hw/vfio/pci.c                                 |  36 ++-
 hw/virtio/virtio-balloon.c                    |  33 ++-
 hw/virtio/virtio-bus.c                        |  16 +-
 hw/virtio/virtio-rng-pci.c                    |   6 +-
 hw/virtio/virtio-rng.c                        |   6 +-
 hw/virtio/virtio.c                            |  17 +-
 hw/watchdog/wdt_aspeed.c                      |   4 +-
 hw/xen/xen-backend.c                          |   6 +-
 hw/xen/xen-bus.c                              |  85 +++----
 hw/xen/xen-host-pci-device.c                  |  26 +--
 hw/xen/xen_pt.c                               |  24 +-
 hw/xen/xen_pt_config_init.c                   |  19 +-
 io/dns-resolver.c                             |   6 +-
 io/net-listener.c                             |   6 +-
 iothread.c                                    |  25 +-
 job.c                                         |   6 +-
 memory.c                                      |  54 ++---
 memory_mapping.c                              |   6 +-
 migration/colo.c                              |  33 +--
 migration/migration.c                         |  35 ++-
 migration/ram.c                               |  12 +-
 migration/rdma.c                              |  11 +-
 migration/socket.c                            |  16 +-
 monitor/hmp-cmds.c                            |   8 +-
 monitor/misc.c                                |   8 +-
 monitor/qmp-cmds.c                            |   6 +-
 net/can/can_host.c                            |   6 +-
 net/dump.c                                    |  14 +-
 net/filter-buffer.c                           |  14 +-
 net/filter.c                                  |   6 +-
 net/net.c                                     |  44 ++--
 net/netmap.c                                  |   6 +-
 net/slirp.c                                   |   6 +-
 net/tap-bsd.c                                 |   1 +
 net/tap-solaris.c                             |   1 +
 net/tap-stub.c                                |   1 +
 net/tap.c                                     |  44 ++--
 qapi/opts-visitor.c                           |   1 +
 qapi/qapi-dealloc-visitor.c                   |   8 +
 qapi/qapi-visit-core.c                        |  53 ++---
 qapi/qmp-dispatch.c                           |   6 +-
 qapi/string-input-visitor.c                   |   6 +-
 qdev-monitor.c                                |  37 ++-
 qga/commands-posix.c                          | 197 ++++++----------
 qga/commands-win32.c                          | 127 ++++------
 qom/object.c                                  | 187 ++++++---------
 qom/object_interfaces.c                       |  26 +--
 qom/qom-qobject.c                             |   6 +-
 replication.c                                 |  24 +-
 scsi/pr-manager-helper.c                      |   6 +-
 stubs/xen-hvm.c                               |   3 +
 target/alpha/cpu.c                            |   6 +-
 target/arm/cpu.c                              |   6 +-
 target/arm/cpu64.c                            |  10 +-
 target/cris/cpu.c                             |   6 +-
 target/hppa/cpu.c                             |   6 +-
 target/i386/cpu.c                             | 107 ++++-----
 target/lm32/cpu.c                             |   6 +-
 target/m68k/cpu.c                             |   6 +-
 target/microblaze/cpu.c                       |   6 +-
 target/mips/cpu.c                             |   6 +-
 target/moxie/cpu.c                            |   6 +-
 target/nios2/cpu.c                            |   6 +-
 target/openrisc/cpu.c                         |   6 +-
 target/ppc/compat.c                           |  18 +-
 target/ppc/kvm.c                              |   6 +-
 target/ppc/translate_init.inc.c               |  23 +-
 target/riscv/cpu.c                            |   6 +-
 target/s390x/cpu.c                            |  25 +-
 target/s390x/kvm-stub.c                       |   1 +
 target/sh4/cpu.c                              |   6 +-
 target/sparc/cpu.c                            |  12 +-
 target/tilegx/cpu.c                           |   6 +-
 target/tricore/cpu.c                          |   6 +-
 target/unicore32/cpu.c                        |   6 +-
 target/xtensa/cpu.c                           |   6 +-
 tests/test-image-locking.c                    |   6 +-
 tests/test-qmp-cmds.c                         |   7 +
 tpm.c                                         |   6 +-
 trace/qmp.c                                   |  12 +-
 ui/input-barrier.c                            |   6 +-
 ui/input.c                                    |  12 +-
 ui/vnc.c                                      |  27 +--
 util/error.c                                  |   8 +-
 util/main-loop.c                              |   4 +-
 util/oslib-posix.c                            |   5 +-
 util/qemu-config.c                            |  27 +--
 util/qemu-option.c                            |  50 ++--
 util/qemu-sockets.c                           |  25 +-
 vl.c                                          |  13 +-
 scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
 319 files changed, 2729 insertions(+), 4245 deletions(-)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

-- 
2.21.0

[RFC v2 1/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
any function with errp parameter.

It has three goals:

1. Fix issue with error_fatal & error_append_hint: user can't see these
hints, because exit() happens in error_setg earlier than hint is
appended. [Reported by Greg Kurz]

2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself don't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
[Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions should be merely updated to
return int error code).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01..f6f4fa0fac 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
                         ErrorClass err_class, const char *fmt, ...)
     GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+    Error *local_err;
+    Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+    error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_FUNCTION_BEGIN
+ *
+ * This macro MUST be the first line of EACH function with Error **errp
+ * parameter.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to
+ * local Error object, which will be automatically propagated to original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to append hint (by error_append_hint)
+ * (as, if it was error_fatal, we swapped it by local_error to be
+ * propagated on cleanup).
+ *
+ * Note: we don't wrap error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_FUNCTION_BEGIN() \
+g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
+Error **__local_errp_unused __attribute__ ((unused)) = \
+    (errp = (errp == NULL || *errp == error_fatal ? \
+             &__auto_errp_prop.local_err : errp))
+
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.
-- 
2.21.0

[RFC v2 2/9] qapi/error: add (Error **errp) cleaning APIs

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qapi/error.h | 22 ++++++++++++++++++++++
 util/error.c         |  6 +++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index f6f4fa0fac..551385aa91 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -297,6 +297,28 @@ void warn_report_err(Error *err);
  */
 void error_report_err(Error *err);
 
+/*
+ * Functions to clean Error **errp: call corresponding Error *err cleaning
+ * function an set pointer to NULL
+ */
+static inline void error_free_errp(Error **errp)
+{
+    error_free(*errp_in);
+    *errp_in = NULL;
+}
+
+static inline void error_report_errp(Error **errp)
+{
+    error_report_err(*errp_in);
+    *errp_in = NULL;
+}
+
+static inline void warn_report_errp(Error **errp)
+{
+    warn_report_err(*errp_in);
+    *errp_in = NULL;
+}
+
 /*
  * Convenience function to error_prepend(), warn_report() and free @err.
  */
diff --git a/util/error.c b/util/error.c
index d4532ce318..dfba091757 100644
--- a/util/error.c
+++ b/util/error.c
@@ -273,9 +273,9 @@ void error_free(Error *err)
 
 void error_free_or_abort(Error **errp)
 {
-    assert(errp && *errp);
-    error_free(*errp);
-    *errp = NULL;
+    assert(errp_in && *errp_in);
+    error_free(*errp_in);
+    *errp_in = NULL;
 }
 
 void error_propagate(Error **dst_errp, Error *local_err)
-- 
2.21.0

[RFC v2 3/9] errp: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

Error **errp is almost always OUT-argument: it's assumed to be NULL, or
pointer to NULL-initialized pointer, or pointer to error_abort or
error_fatal, for callee to report error.

But very few functions (most of the are error API) instead get Error
**errp as IN-argument: it's assumed to be set, and callee should clean
it. In such cases, rename errp to errp_in.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/monitor/hmp.h |  2 +-
 include/qapi/error.h  |  8 ++++----
 ui/vnc.h              |  2 +-
 monitor/hmp-cmds.c    |  8 ++++----
 ui/vnc.c              | 10 +++++-----
 util/error.c          |  2 +-
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index a0e9511440..f929814f1a 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -16,7 +16,7 @@
 
 #include "qemu/readline.h"
 
-void hmp_handle_error(Monitor *mon, Error **errp);
+void hmp_handle_error(Monitor *mon, Error **errp_in);
 
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 551385aa91..4264d22223 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -283,7 +283,7 @@ void error_free(Error *err);
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-void error_free_or_abort(Error **errp);
+void error_free_or_abort(Error **errp_in);
 
 /*
  * Convenience function to warn_report() and free @err.
@@ -301,19 +301,19 @@ void error_report_err(Error *err);
  * Functions to clean Error **errp: call corresponding Error *err cleaning
  * function an set pointer to NULL
  */
-static inline void error_free_errp(Error **errp)
+static inline void error_free_errp(Error **errp_in)
 {
     error_free(*errp_in);
     *errp_in = NULL;
 }
 
-static inline void error_report_errp(Error **errp)
+static inline void error_report_errp(Error **errp_in)
 {
     error_report_err(*errp_in);
     *errp_in = NULL;
 }
 
-static inline void warn_report_errp(Error **errp)
+static inline void warn_report_errp(Error **errp_in)
 {
     warn_report_err(*errp_in);
     *errp_in = NULL;
diff --git a/ui/vnc.h b/ui/vnc.h
index fea79c2fc9..00e0b48f2f 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
 
 /* Protocol stage functions */
 void vnc_client_error(VncState *vs);
-size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
+size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
 
 void start_client_init(VncState *vs);
 void start_auth_vnc(VncState *vs);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..941d5d0a45 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -60,11 +60,11 @@
 #include <spice/enums.h>
 #endif
 
-void hmp_handle_error(Monitor *mon, Error **errp)
+void hmp_handle_error(Monitor *mon, Error **errp_in)
 {
-    assert(errp);
-    if (*errp) {
-        error_reportf_err(*errp, "Error: ");
+    assert(errp_in);
+    if (*errp_in) {
+        error_reportf_err(*errp_in, "Error: ");
     }
 }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 87b8045afe..9d6384d9b1 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
     g_free(vs);
 }
 
-size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
+size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
 {
     if (ret <= 0) {
         if (ret == 0) {
@@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
             vnc_disconnect_start(vs);
         } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
             trace_vnc_client_io_error(vs, vs->ioc,
-                                      errp ? error_get_pretty(*errp) :
+                                      errp_in ? error_get_pretty(*errp_in) :
                                       "Unknown");
             vnc_disconnect_start(vs);
         }
 
-        if (errp) {
-            error_free(*errp);
-            *errp = NULL;
+        if (errp_in) {
+            error_free(*errp_in);
+            *errp_in = NULL;
         }
         return 0;
     }
diff --git a/util/error.c b/util/error.c
index dfba091757..b3ff3832d6 100644
--- a/util/error.c
+++ b/util/error.c
@@ -271,7 +271,7 @@ void error_free(Error *err)
     }
 }
 
-void error_free_or_abort(Error **errp)
+void error_free_or_abort(Error **errp_in)
 {
     assert(errp_in && *errp_in);
     error_free(*errp_in);
-- 
2.21.0

[RFC v2 4/9] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Vladimir Sementsov-Ogievskiy]

fit_load_fdt forget to zero errp. Fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/core/loader-fit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..fe5bcc6700 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -200,7 +200,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
     err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
     if (err == -ENOENT) {
         load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
-        error_free(*errp);
+        error_free_errp(errp);
     } else if (err) {
         error_prepend(errp, "unable to read FDT load address from FIT: ");
         ret = err;
-- 
2.21.0

[RFC v2 5/9] net/net: fix local variable shadowing in net_client_init

[Vladimir Sementsov-Ogievskiy]

Don't shadow Error *err: it's a bad thing. This patch also simplifies
following Error propagation conversion.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 net/net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/net.c b/net/net.c
index 84aa6d8d00..5fc72511c1 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1128,10 +1128,10 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
 
             if (substrings[1]) {
                 /* User-specified prefix length.  */
-                int err;
+                int ret2;
 
-                err = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
-                if (err) {
+                ret2 = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
+                if (ret2) {
                     error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                                "ipv6-prefixlen", "a number");
                     goto out;
-- 
2.21.0

[RFC v2 6/9] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 0000000000..1a3f006f0b
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,82 @@
+@@
+identifier fn;
+identifier local_err;
+@@
+
+ fn(..., Error **errp)
+ {
++    ERRP_FUNCTION_BEGIN();
+ }
+
+@rule1@
+identifier fn;
+identifier local_err;
+@@
+
+ fn(..., Error **errp)
+ {
+     <...
+-    Error *local_err = NULL;
+     ...>
+ }
+
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+identifier out;
+@@
+
+ fn(...)
+ {
+     <...
+-    goto out;
++    return;
+     ...>
+- out:
+-    error_propagate(errp, local_err);
+ }
+
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    error_free(local_err);
+-    local_err = NULL;
++    error_free_errp(errp);
+|
+-    error_free(local_err);
++    error_free_errp(errp);
+|
+-    error_report_err(local_err);
++    error_report_errp(errp);
+|
+-    warn_report_err(local_err);
++    warn_report_errp(errp);
+|
+-    error_propagate(errp, local_err);
+)
+     ...>
+ }
+
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    &local_err
++    errp
+|
+-    local_err
++    *errp
+)
+     ...>
+ }
-- 
2.21.0

[RFC v2 8/9] fix-compilation: empty goto

[Vladimir Sementsov-Ogievskiy]

I don't understand, why these special cases are not handled by
coccinelle. Possibly, it's a bug in coccinelle itself.

If no other ideas, these fixes may be just merged to autogenerated
commit(commits), with appropriate notice in commit message.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 backends/cryptodev.c    | 7 -------
 hw/dma/xilinx_axidma.c  | 7 ++-----
 hw/net/xilinx_axienet.c | 7 ++-----
 hw/ppc/spapr.c          | 3 +--
 hw/ppc/spapr_cpu_core.c | 3 +--
 hw/s390x/s390-ccw.c     | 3 +--
 hw/vfio/ap.c            | 3 +--
 hw/vfio/ccw.c           | 3 +--
 8 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index dcdb8481b2..aa4a62ace0 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -176,14 +176,7 @@ cryptodev_backend_complete(UserCreatable *uc, Error **errp)
 
     if (bc->init) {
         bc->init(backend, errp);
-        if (*errp) {
-            goto out;
-        }
     }
-
-    return;
-
-out:
 }
 
 void cryptodev_backend_set_used(CryptoDevBackend *backend, bool used)
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index b5f49e6f88..b5456f5db2 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -536,12 +536,12 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
                              OBJ_PROP_LINK_STRONG,
                              errp);
     if (*errp) {
-        goto xilinx_axidma_realize_fail;
+        return;
     }
     object_property_set_link(OBJECT(ds), OBJECT(s), "dma", errp);
     object_property_set_link(OBJECT(cs), OBJECT(s), "dma", errp);
     if (*errp) {
-        goto xilinx_axidma_realize_fail;
+        return;
     }
 
     int i;
@@ -554,9 +554,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
         st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
         ptimer_set_freq(st->ptimer, s->freqhz);
     }
-    return;
-
-xilinx_axidma_realize_fail:
 }
 
 static void xilinx_axidma_init(Object *obj)
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 5f87a6336b..372ca88956 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -963,12 +963,12 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
                              OBJ_PROP_LINK_STRONG,
                              errp);
     if (*errp) {
-        goto xilinx_enet_realize_fail;
+        return;
     }
     object_property_set_link(OBJECT(ds), OBJECT(s), "enet", errp);
     object_property_set_link(OBJECT(cs), OBJECT(s), "enet", errp);
     if (*errp) {
-        goto xilinx_enet_realize_fail;
+        return;
     }
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
@@ -982,9 +982,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
     s->TEMAC.parent = s;
 
     s->rxmem = g_malloc(s->c_rxmem);
-    return;
-
-xilinx_enet_realize_fail:
 }
 
 static void xilinx_enet_init(Object *obj)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5ef043bd72..7b0ab71f23 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3503,7 +3503,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     pc_dimm_plug(dimm, MACHINE(ms), errp);
     if (*errp) {
-        goto out;
+        return;
     }
 
     addr = object_property_get_uint(OBJECT(dimm),
@@ -3522,7 +3522,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
 out_unplug:
     pc_dimm_unplug(dimm, MACHINE(ms));
-out:
 }
 
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 93f9b0e0c9..c35696ff42 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -224,7 +224,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
     object_property_set_bool(OBJECT(cpu), true, "realized", errp);
     if (*errp) {
-        goto error;
+        return;
     }
 
     /* Set time-base frequency to 512 MHz */
@@ -251,7 +251,6 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
 error_unregister:
     qemu_unregister_reset(spapr_cpu_reset, cpu);
     cpu_remove_sync(CPU(cpu));
-error:
 }
 
 static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 0ae1951507..8695d2c445 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -94,7 +94,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
 
     s390_ccw_get_dev_info(cdev, sysfsdev, errp);
     if (*errp) {
-        goto out_err_propagate;
+        return;
     }
 
     sch = css_create_sch(ccw_dev->devno, errp);
@@ -127,7 +127,6 @@ out_err:
     g_free(sch);
 out_mdevid_free:
     g_free(cdev->mdevid);
-out_err_propagate:
 }
 
 static void s390_ccw_unrealize(S390CCWDevice *cdev, Error **errp)
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index e072a08f1d..8fbaa724c2 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -95,7 +95,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
 
     vfio_group = vfio_ap_get_group(vapdev, errp);
     if (!vfio_group) {
-        goto out_err;
+        return;
     }
 
     vapdev->vdev.ops = &vfio_ap_ops;
@@ -122,7 +122,6 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
 out_get_dev_err:
     vfio_ap_put_device(vapdev);
     vfio_put_group(vfio_group);
-out_err:
 }
 
 static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index d2b526667e..c24ffdb3af 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -486,7 +486,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
     if (cdc->realize) {
         cdc->realize(cdev, vcdev->vdev.sysfsdev, errp);
         if (*errp) {
-            goto out_err_propagate;
+            return;
         }
     }
 
@@ -522,7 +522,6 @@ out_group_err:
     if (cdc->unrealize) {
         cdc->unrealize(cdev, NULL);
     }
-out_err_propagate:
 }
 
 static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
-- 
2.21.0

[RFC v2 9/9] fix-compilation: includes

[Vladimir Sementsov-Ogievskiy]

Hmm. Should we allow empty stubs with errp parameter without calling
new macro?

Or, just apply this commit before auto-generated commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 target/ppc/kvm_ppc.h        | 2 ++
 target/s390x/cpu_models.h   | 1 +
 hw/i386/kvm/apic.c          | 1 +
 hw/misc/arm11scu.c          | 1 +
 hw/misc/mps2-scc.c          | 1 +
 hw/pci-bridge/dec.c         | 1 +
 qapi/qapi-dealloc-visitor.c | 1 +
 stubs/xen-hvm.c             | 1 +
 8 files changed, 9 insertions(+)

diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 807c245e90..f6366c19aa 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -9,6 +9,8 @@
 #ifndef KVM_PPC_H
 #define KVM_PPC_H
 
+#include "qapi/error.h"
+
 #define TYPE_HOST_POWERPC_CPU POWERPC_CPU_TYPE_NAME("host")
 
 #ifdef CONFIG_KVM
diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h
index 5329045a71..062161c5fa 100644
--- a/target/s390x/cpu_models.h
+++ b/target/s390x/cpu_models.h
@@ -16,6 +16,7 @@
 #include "cpu_features.h"
 #include "gen-features.h"
 #include "hw/core/cpu.h"
+#include "qapi/error.h"
 
 /* static CPU definition */
 struct S390CPUDef {
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index d125b370f4..cadf75b71c 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -18,6 +18,7 @@
 #include "sysemu/hw_accel.h"
 #include "sysemu/kvm.h"
 #include "target/i386/kvm_i386.h"
+#include "qapi/error.h"
 
 static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
                                     int reg_id, uint32_t val)
diff --git a/hw/misc/arm11scu.c b/hw/misc/arm11scu.c
index befc85f321..9c1fec7825 100644
--- a/hw/misc/arm11scu.c
+++ b/hw/misc/arm11scu.c
@@ -13,6 +13,7 @@
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 
 static uint64_t mpcore_scu_read(void *opaque, hwaddr offset,
                                 unsigned size)
diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
index f2a00d3235..c41e776996 100644
--- a/hw/misc/mps2-scc.c
+++ b/hw/misc/mps2-scc.c
@@ -26,6 +26,7 @@
 #include "hw/registerfields.h"
 #include "hw/misc/mps2-scc.h"
 #include "hw/qdev-properties.h"
+#include "qapi/error.h"
 
 REG32(CFG0, 0)
 REG32(CFG1, 4)
diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index fbe781474e..06445e0545 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -31,6 +31,7 @@
 #include "hw/pci/pci_host.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_bus.h"
+#include "qapi/error.h"
 
 /* debug DEC */
 //#define DEBUG_DEC
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 3ee4c7a2e7..e265e29234 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -16,6 +16,7 @@
 #include "qapi/dealloc-visitor.h"
 #include "qapi/qmp/qnull.h"
 #include "qapi/visitor-impl.h"
+#include "qapi/error.h"
 
 struct QapiDeallocVisitor
 {
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 840a2a0d09..350c762c64 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -12,6 +12,7 @@
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/error.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {
-- 
2.21.0

Re: [RFC v2 3/9] errp: rename errp to errp_in where it is IN-argument

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
> pointer to NULL-initialized pointer, or pointer to error_abort or
> error_fatal, for callee to report error.
> 
> But very few functions (most of the are error API) instead get Error
> **errp as IN-argument: it's assumed to be set, and callee should clean
> it. In such cases, rename errp to errp_in.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/monitor/hmp.h |  2 +-
>  include/qapi/error.h  |  8 ++++----
>  ui/vnc.h              |  2 +-
>  monitor/hmp-cmds.c    |  8 ++++----
>  ui/vnc.c              | 10 +++++-----
>  util/error.c          |  2 +-
>  6 files changed, 16 insertions(+), 16 deletions(-)

This is worthwhile, regardless of the fate of the rest of the series.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 2/9] qapi/error: add (Error **errp) cleaning APIs

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

A "why" as the commit message body wouldn't hurt.

>  include/qapi/error.h | 22 ++++++++++++++++++++++
>  util/error.c         |  6 +++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f6f4fa0fac..551385aa91 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -297,6 +297,28 @@ void warn_report_err(Error *err);
>   */
>  void error_report_err(Error *err);
>  
> +/*
> + * Functions to clean Error **errp: call corresponding Error *err cleaning
> + * function an set pointer to NULL

s/an/and/

> + */
> +static inline void error_free_errp(Error **errp)
> +{
> +    error_free(*errp_in);

Fails to compile.  Did you mean for this to come after 3/9?

> +    *errp_in = NULL;
> +}
> +
> +static inline void error_report_errp(Error **errp)
> +{
> +    error_report_err(*errp_in);
> +    *errp_in = NULL;
> +}
> +
> +static inline void warn_report_errp(Error **errp)
> +{
> +    warn_report_err(*errp_in);
> +    *errp_in = NULL;
> +}
> +
>  /*
>   * Convenience function to error_prepend(), warn_report() and free @err.
>   */
> diff --git a/util/error.c b/util/error.c
> index d4532ce318..dfba091757 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -273,9 +273,9 @@ void error_free(Error *err)
>  
>  void error_free_or_abort(Error **errp)
>  {
> -    assert(errp && *errp);
> -    error_free(*errp);
> -    *errp = NULL;
> +    assert(errp_in && *errp_in);
> +    error_free(*errp_in);
> +    *errp_in = NULL;

Did you mean to use error_free_errp() instead of these last two lines?

>  }
>  
>  void error_propagate(Error **dst_errp, Error *local_err)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 4/9] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> fit_load_fdt forget to zero errp. Fix it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  hw/core/loader-fit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Independent bug fix.  Either we take the (fixed) 2-3 (to rely on the new
error_free_errp), or you could open-code the assignment of errp=NULL to
take this on its own, regardless of the hfate of the rest of the series.

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..fe5bcc6700 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -200,7 +200,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>      err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>      if (err == -ENOENT) {
>          load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> -        error_free(*errp);
> +        error_free_errp(errp);
>      } else if (err) {
>          error_prepend(errp, "unable to read FDT load address from FIT: ");
>          ret = err;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 5/9] net/net: fix local variable shadowing in net_client_init

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Don't shadow Error *err: it's a bad thing. This patch also simplifies
> following Error propagation conversion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  net/net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Can be applied independently of the rest of this series.

> 
> diff --git a/net/net.c b/net/net.c
> index 84aa6d8d00..5fc72511c1 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1128,10 +1128,10 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
>  
>              if (substrings[1]) {
>                  /* User-specified prefix length.  */
> -                int err;
> +                int ret2;
>  
> -                err = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
> -                if (err) {
> +                ret2 = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
> +                if (ret2) {

In fact, you don't need ret2; you could just:

if (qemu_strtoul(...)) {

at which point you could then join:

if (substrings[1] &&
    qemus_strtoul(...)) {

>                      error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                                 "ipv6-prefixlen", "a number");
>                      goto out;
> 

Either way,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 0/9] error: auto propagated local_err

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is a proposal of auto propagation for local_err, to not call
> error_propagate on every exit point, when we deal with local_err.
> 
> It also fixes two issues:
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself don't fix the issue, but it allows to [3.] drop all

doesn't

> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
> 
> It's still an RFC, due to the following reasons:
> 
> 1. I'm new to coccinella, so I failed to do the following pattern:
> 
>  <...
> - goto out;
> + return;
>  ...>
> - out:
> - error_propagate(errp, local_err)
> 
> So, here is compilation fix 08.. Who can help with it? If nobody, 08 is
> to be merged to 07 by hand.

I'm not sure either; but I agree that if we can't figure out how to make
Coccinelle do quite what we want, that we are better off squashing in
compile fixes.

Also, while I like Coccinelle for automating the conversion, it's harder
to get everyone to run it; it would be nice if we could also figure out
a patch to scripts/checkpatch.pl that for any instance of 'Error
**errp)\n{\n' not followed by either } or the new macro, we flag that as
a checkpatch warning or error.

> 
> 2. Question about using new macro in empty stub functions - see 09

It would be nice if we could exempt empty functions - no need to use the
macro if there is no function body otherwise.  I'm not sure if
Coccinelle can do that filtering during the conversion, or if we clean
up by hand after the fact.

> 
> 3. What to do with huge auto-generated commit 07? Should I split it
> per-maintainer or per-subsystem, or leave it as-is?

It's big. I'd split it into multiple patches (and reduce the cc - except
for the cover letter, the rest of the patches can be limited to the
actual maintainer/subsystem affected rather than everyone involved
anywhere else in the series. With the current large cc, anyone that
replies gets several mail bounces about "too many recipients").  It may
be easier to split along directory boundaries than by maintainer
boundaries.  Markus has applied large tree-wide Coccinelle cleanups
before, maybe he has some advice.

> 
> 4. Also, checkpatch has some complains about 07 patch:
>   - using tabs.. (hmm exactly stubs functions..)
>   - empty ifs
>   Again, I don't see any ways to fix it other that by hand and merge to
>   07..

Hand cleanups for formatting or compilation fixes to Coccinelle's work
is not an uncommon issue after large patches; thankfully it's also not
very difficult (and surprisingly needed in very few places compared to
how much actually gets touched).

> 
> ==================
> 
> Also, if we decide, that this all is too huge, here is plan B:
> 
> 1. apply 01
> 2. fix only functions that don't use local_err and use
> error_append_hint, by just invocation of new macro at function start -
> it will substitute Greg's series with no pain.
> 3[optional]. Do full update for some subsystems, for example, only for
> block* and nbd*

Even if we go with plan B, it's still worth checking in a Coccinelle
script that we can periodically run to make sure we aren't missing out
on the use of the macro where it is needed.

> 
> Vladimir Sementsov-Ogievskiy (9):
>   error: auto propagated local_err
>   qapi/error: add (Error **errp) cleaning APIs
>   errp: rename errp to errp_in where it is IN-argument
>   hw/core/loader-fit: fix freeing errp in fit_load_fdt
>   net/net: fix local variable shadowing in net_client_init
>   scripts: add coccinelle script to use auto propagated errp
>   Use auto-propagated errp
>   fix-compilation: empty goto
>   fix-compilation: includes
> 
>  include/hw/pci-host/spapr.h                   |   2 +
>  include/monitor/hmp.h                         |   2 +-
>  include/qapi/error.h                          |  61 ++++-
>  target/ppc/kvm_ppc.h                          |   3 +
>  target/s390x/cpu_models.h                     |   3 +
>  ui/vnc.h                                      |   2 +-

>  vl.c                                          |  13 +-
>  scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>  319 files changed, 2729 insertions(+), 4245 deletions(-)
>  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

The diffstat is huge, but promising.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 1/9] error: auto propagated local_err

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> any function with errp parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself don't fix the issue, but it allows to [3.] drop all

doesn't

> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
> 
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions should be merely updated to

Maybe:

s/should/could/

> return int error code).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..f6f4fa0fac 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
>                          ErrorClass err_class, const char *fmt, ...)
>      GCC_FMT_ATTR(6, 7);
>  
> +typedef struct ErrorPropagator {
> +    Error *local_err;
> +    Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +    error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_FUNCTION_BEGIN
> + *
> + * This macro MUST be the first line of EACH function with Error **errp
> + * parameter.

Maybe s/EACH function/ANY non-empty function/ (allowing our stub
functions to be exemptions).

> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to

s/to/to a/

> + * local Error object, which will be automatically propagated to original

s/to/to the/

> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to append hint (by error_append_hint)

s/append hint/append hints/

> + * (as, if it was error_fatal, we swapped it by local_error to be

s/by local_error/with a local error/

> + * propagated on cleanup).
> + *
> + * Note: we don't wrap error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.

s/wrap/wrap the/

> + */
> +#define ERRP_FUNCTION_BEGIN() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +Error **__local_errp_unused __attribute__ ((unused)) = \
> +    (errp = (errp == NULL || *errp == error_fatal ? \
> +             &__auto_errp_prop.local_err : errp))

I'm not sold on why we need the dummy declaration (yeah, I know it's so
that you don't have to fight the battle of mixing declarations and
statements - but this is in a macro call whose usage LOOKS like a
statement rather than a declaration, so we're already on fuzzy ground).
We could make this macro expansion one line shorter and still be
correct, but I'm not going to insist (we'll see what consensus is,
and/or what Markus says).

> +
> +
>  /*
>   * Special error destination to abort on error.
>   * See error_setg() and error_propagate() for details.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 6/9] scripts: add coccinelle script to use auto propagated errp

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
> 
> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
> new file mode 100644
> index 0000000000..1a3f006f0b
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,82 @@
> +@@
> +identifier fn;
> +identifier local_err;
> +@@
> +
> + fn(..., Error **errp)
> + {
> ++    ERRP_FUNCTION_BEGIN();
> + }

This doesn't catch functions where Error **errp is not the last
parameter.  Some examples (some of which may need independent tweaking
in their own right for being inconsistent, although we DO want errp to
appear before any 'format, ...' arguments):

block/ssh.c:sftp_error_setg(Error **errp, BDRVSSHState *s, const char
*fs, ...)
exec.c:static void ram_block_add(RAMBlock *new_block, Error **errp, bool
shared)

Does running this Coccinelle script 2 times in a row add a second
ERRP_FUNCTION_BEGIN() line?  We want it to be idempotent (no changes on
a second run).  (Admittedly, I did not actually test that yet).  Also, I
don't know if this can be tweaked to avoid adding the line to a function
with an empty body, maybe:

 fn(..., Error **errp, ...)
 {
+    ERRP_FUNCTION_BEGIN();
     ...
 }

to only add it to a function that already has a body (thanks to the ...)
- but I'm fuzzy enough on Coccinelle that I may be saying something
totally wrong.

> +
> +@rule1@
> +identifier fn;
> +identifier local_err;
> +@@
> +
> + fn(..., Error **errp)
> + {
> +     <...
> +-    Error *local_err = NULL;
> +     ...>
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +identifier out;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +-    goto out;
> ++    return;
> +     ...>
> +- out:
> +-    error_propagate(errp, local_err);
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    error_free(local_err);
> +-    local_err = NULL;
> ++    error_free_errp(errp);
> +|
> +-    error_free(local_err);
> ++    error_free_errp(errp);
> +|
> +-    error_report_err(local_err);
> ++    error_report_errp(errp);
> +|
> +-    warn_report_err(local_err);
> ++    warn_report_errp(errp);
> +|
> +-    error_propagate(errp, local_err);
> +)
> +     ...>
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    &local_err
> ++    errp
> +|
> +-    local_err
> ++    *errp
> +)
> +     ...>
> + }
> 

Overall, the script makes sense in my reading (but no idea if it
actually catches everything we want, or if it missed something).  At any
rate, once patch 7 is split into more manageable chunks, we can
definitely spot-check results to make sure they all look reasonable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 7/9] Use auto-propagated errp

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> This commit is generated by command
> 
> git grep -l 'Error \*\*errp' | while read f; \
> do spatch --sp-file \
> scripts/coccinelle/auto-propagated-errp.cocci --in-place $f; done
> 

As mentioned in your cover letter, this fails syntax-check and
compilation without squashing in some followups; if we can't improve the
.cocci script to do it automatically, then manually squashing in
cleanups (and documenting what types of cleanups they were) is fine.
(The goal for a mechanical patch like this is to make it easy enough to
automate downstream, even where the file contents are changed, but where
the process for creating those changes are the same).

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Spot-checking

>  block/io.c                          |  11 +-

>  block/nbd.c                         |  44 +++---

>  qapi/qapi-visit-core.c              |  53 ++-----

just to see how it looks.

> +++ b/block/io.c
> @@ -136,7 +136,6 @@ static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)
>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    Error *local_err = NULL;
>  

Umm, no insertion of ERR_FUNCTION_BEGIN().  Oops.

>      memset(&bs->bl, 0, sizeof(bs->bl));
>  
> @@ -151,9 +150,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>  
>      /* Take some limits from the children as a default */
>      if (bs->file) {
> -        bdrv_refresh_limits(bs->file->bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        bdrv_refresh_limits(bs->file->bs, errp);
> +        if (*errp) {
>              return;
>          }
>          bdrv_merge_limits(&bs->bl, &bs->file->bs->bl);
> @@ -166,9 +164,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
>  
>      if (bs->backing) {
> -        bdrv_refresh_limits(bs->backing->bs, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        bdrv_refresh_limits(bs->backing->bs, errp);
> +        if (*errp) {
>              return;

Rest of the changes in this file are good if the macro gets added correctly.

>          }
>          bdrv_merge_limits(&bs->bl, &bs->backing->bs->bl);

> +++ b/block/nbd.c
> @@ -808,7 +808,6 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle,
>      NBDReplyChunkIter iter;
>      NBDReply reply;
>      void *payload = NULL;
> -    Error *local_err = NULL;

Recurring problem of not inserting the macro as expected.

>  
>      NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
>                              qiov, &reply, &payload)
> @@ -827,20 +826,20 @@ static int nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t handle,
>              break;
>          case NBD_REPLY_TYPE_OFFSET_HOLE:
>              ret = nbd_parse_offset_hole_payload(s, &reply.structured, payload,
> -                                                offset, qiov, &local_err);
> +                                                offset, qiov, errp);
>              if (ret < 0) {
>                  nbd_channel_error(s, ret);
> -                nbd_iter_channel_error(&iter, ret, &local_err);
> +                nbd_iter_channel_error(&iter, ret, errp);
>              }
>              break;
>          default:
>              if (!nbd_reply_type_is_error(chunk->type)) {
>                  /* not allowed reply type */
>                  nbd_channel_error(s, -EINVAL);
> -                error_setg(&local_err,
> +                error_setg(errp,
>                             "Unexpected reply type: %d (%s) for CMD_READ",

Could almost fold these lines (but I'm not asking you to; keeping this
patch as mechanical as possible is fine).

>                             chunk->type, nbd_reply_type_lookup(chunk->type));
> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
> +                nbd_iter_channel_error(&iter, -EINVAL, errp);
>              }
>          }
>  
> @@ -861,7 +860,6 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>      NBDReplyChunkIter iter;
>      NBDReply reply;
>      void *payload = NULL;
> -    Error *local_err = NULL;
>      bool received = false;

Oops on the macro.

> @@ -1174,15 +1172,13 @@ static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
>                                                    Error **errp)
>  {
>      QIOChannelSocket *sioc;
> -    Error *local_err = NULL;
>  
>      sioc = qio_channel_socket_new();
>      qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client");
>  
> -    qio_channel_socket_connect_sync(sioc, saddr, &local_err);
> -    if (local_err) {
> +    qio_channel_socket_connect_sync(sioc, saddr, errp);
> +    if (*errp) {
>          object_unref(OBJECT(sioc));
> -        error_propagate(errp, local_err);
>          return NULL;

But getting rid of error_propagate() is nice.

Did you grep for instances of error_propagate() after your mechanical
patch, to see what else the Coccinelle script might have missed?


> +++ b/qapi/opts-visitor.c
> @@ -275,6 +275,7 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
>  static void
>  opts_check_list(Visitor *v, Error **errp)
>  {
> +	ERRP_FUNCTION_BEGIN();
>      /*
>       * Unvisited list elements will be reported later when checking
>       * whether unvisited struct members remain.

Here the macro got added, but with no obvious benefit later on (although
we also argued that adding it even when it makes no difference is not
bad, if that's easier to automate for style checking).

> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index d192724b13..3ee4c7a2e7 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -25,6 +25,7 @@ struct QapiDeallocVisitor
>  static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
>                                        size_t unused, Error **errp)
>  {
> +	ERRP_FUNCTION_BEGIN();
>  }

Here's an example where exempting empty functions would be nicer.


> +++ b/qapi/qapi-visit-core.c
> @@ -39,18 +39,15 @@ void visit_free(Visitor *v)
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp)
>  {
> -    Error *err = NULL;
> -

Oops, macro not added.

>      trace_visit_start_struct(v, name, obj, size);
>      if (obj) {
>          assert(size);
>          assert(!(v->type & VISITOR_OUTPUT) || *obj);
>      }
> -    v->start_struct(v, name, obj, size, &err);
> +    v->start_struct(v, name, obj, size, errp);
>      if (obj && (v->type & VISITOR_INPUT)) {
> -        assert(!err != !*obj);
> +        assert(!*errp != !*obj);
>      }
> -    error_propagate(errp, err);
>  }

But the cleanup is sane, once the macro is present.

> @@ -152,12 +143,10 @@ void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
>  static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
>                               uint64_t max, const char *type, Error **errp)
>  {
> -    Error *err = NULL;
>      uint64_t value = *obj;
>  
> -    v->type_uint64(v, name, &value, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    v->type_uint64(v, name, &value, errp);
> +    if (*errp) {
>      } else if (value > max) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     name ? name : "null", type);

Results in an empty if which looks funny.  This one could be a manual
touchup later.

> @@ -211,12 +200,10 @@ static void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
>                              int64_t min, int64_t max, const char *type,
>                              Error **errp)
>  {
> -    Error *err = NULL;
>      int64_t value = *obj;
>  
> -    v->type_int64(v, name, &value, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    v->type_int64(v, name, &value, errp);
> +    if (*errp) {
>      } else if (value < min || value > max) {

and again

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 6/9] scripts: add coccinelle script to use auto propagated errp

[Eric Blake]

On 9/23/19 3:05 PM, Eric Blake wrote:

> Does running this Coccinelle script 2 times in a row add a second
> ERRP_FUNCTION_BEGIN() line?  We want it to be idempotent (no changes on
> a second run).  (Admittedly, I did not actually test that yet).  Also, I
> don't know if this can be tweaked to avoid adding the line to a function
> with an empty body, maybe:
> 
>  fn(..., Error **errp, ...)
>  {
> +    ERRP_FUNCTION_BEGIN();
>      ...
>  }

Also untested:

 fn(..., Error **errp, ...)
 {
(
|
     ERRP_FUNCTION_BEGIN();
     ...
|
+    ERRP_FUNCTION_BEGIN()
     ...
)
 }


> Overall, the script makes sense in my reading (but no idea if it
> actually catches everything we want, or if it missed something).

Having spot-checked 7, it definitely misses cases where it was supposed
to add ERRP_FUNCTION_BEGIN().

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 7/9] Use auto-propagated errp

[Vladimir Sementsov-Ogievskiy]

23.09.2019 23:30, Eric Blake wrote:
> On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> This commit is generated by command
>>
>> git grep -l 'Error \*\*errp' | while read f; \
>> do spatch --sp-file \
>> scripts/coccinelle/auto-propagated-errp.cocci --in-place $f; done
>>
> 
> As mentioned in your cover letter, this fails syntax-check and
> compilation without squashing in some followups; if we can't improve the
> .cocci script to do it automatically, then manually squashing in
> cleanups (and documenting what types of cleanups they were) is fine.
> (The goal for a mechanical patch like this is to make it easy enough to
> automate downstream, even where the file contents are changed, but where
> the process for creating those changes are the same).
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> 
> Spot-checking
> 
>>   block/io.c                          |  11 +-
> 
>>   block/nbd.c                         |  44 +++---
> 
>>   qapi/qapi-visit-core.c              |  53 ++-----
> 
> just to see how it looks.
> 
>> +++ b/block/io.c
>> @@ -136,7 +136,6 @@ static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)
>>   void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>>   {
>>       BlockDriver *drv = bs->drv;
>> -    Error *local_err = NULL;
>>   
> 
> Umm, no insertion of ERR_FUNCTION_BEGIN().  Oops.


Oops. Seems I injected it _only_ to empty functions. It's because I missed '...' in first hunk.

-- 
Best regards,
Vladimir

Re: [RFC v2 6/9] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

24.09.2019 0:29, Eric Blake wrote:
> On 9/23/19 3:05 PM, Eric Blake wrote:
> 
>> Does running this Coccinelle script 2 times in a row add a second
>> ERRP_FUNCTION_BEGIN() line?  We want it to be idempotent (no changes on
>> a second run).  (Admittedly, I did not actually test that yet).  Also, I
>> don't know if this can be tweaked to avoid adding the line to a function
>> with an empty body, maybe:
>>
>>   fn(..., Error **errp, ...)
>>   {
>> +    ERRP_FUNCTION_BEGIN();
>>       ...
>>   }

No, we need exactly this to match not only empty functions. But with ... it matches
empty functions as well.

> 
> Also untested:
> 
>   fn(..., Error **errp, ...)
>   {
> (
> |
>       ERRP_FUNCTION_BEGIN();
>       ...
> |
> +    ERRP_FUNCTION_BEGIN()
>       ...
> )
>   }

Seems, that doesn't work..

It says:
12: no available token to attach to

where 12 is line "+    ERRP_FUNCTION_BEGIN()"


So, I tend to just add chunk to remove duplicated invocation :)

> 
> 
>> Overall, the script makes sense in my reading (but no idea if it
>> actually catches everything we want, or if it missed something).
> 
> Having spot-checked 7, it definitely misses cases where it was supposed
> to add ERRP_FUNCTION_BEGIN().
> 


-- 
Best regards,
Vladimir

Re: [RFC v2 0/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

23.09.2019 22:47, Eric Blake wrote:
> On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a proposal of auto propagation for local_err, to not call
>> error_propagate on every exit point, when we deal with local_err.
>>
>> It also fixes two issues:
>> 1. Fix issue with error_fatal & error_append_hint: user can't see these
>> hints, because exit() happens in error_setg earlier than hint is
>> appended. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself don't fix the issue, but it allows to [3.] drop all
> 
> doesn't
> 
>> local_err+error_propagate pattern, which will definitely fix the issue)
>> [Reported by Kevin Wolf]
>>
>> It's still an RFC, due to the following reasons:
>>
>> 1. I'm new to coccinella, so I failed to do the following pattern:
>>
>>   <...
>> - goto out;
>> + return;
>>   ...>
>> - out:
>> - error_propagate(errp, local_err)
>>
>> So, here is compilation fix 08.. Who can help with it? If nobody, 08 is
>> to be merged to 07 by hand.
> 
> I'm not sure either; but I agree that if we can't figure out how to make
> Coccinelle do quite what we want, that we are better off squashing in
> compile fixes.
> 
> Also, while I like Coccinelle for automating the conversion, it's harder
> to get everyone to run it; it would be nice if we could also figure out
> a patch to scripts/checkpatch.pl that for any instance of 'Error
> **errp)\n{\n' not followed by either } or the new macro, we flag that as
> a checkpatch warning or error.
> 
>>
>> 2. Question about using new macro in empty stub functions - see 09
> 
> It would be nice if we could exempt empty functions - no need to use the
> macro if there is no function body otherwise.  I'm not sure if
> Coccinelle can do that filtering during the conversion, or if we clean
> up by hand after the fact.
> 
>>
>> 3. What to do with huge auto-generated commit 07? Should I split it
>> per-maintainer or per-subsystem, or leave it as-is?
> 
> It's big. I'd split it into multiple patches (and reduce the cc - except
> for the cover letter, the rest of the patches can be limited to the
> actual maintainer/subsystem affected rather than everyone involved
> anywhere else in the series. With the current large cc, anyone that
> replies gets several mail bounces about "too many recipients").  It may
> be easier to split along directory boundaries than by maintainer
> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
> before, maybe he has some advice.


If split by subsystem it would be 200+ patches:
git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
205


Try to look at larger subsystem:
git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
139

still too many.. Or is it OK?


> 
>>
>> 4. Also, checkpatch has some complains about 07 patch:
>>    - using tabs.. (hmm exactly stubs functions..)
>>    - empty ifs
>>    Again, I don't see any ways to fix it other that by hand and merge to
>>    07..
> 
> Hand cleanups for formatting or compilation fixes to Coccinelle's work
> is not an uncommon issue after large patches; thankfully it's also not
> very difficult (and surprisingly needed in very few places compared to
> how much actually gets touched).
> 
>>
>> ==================
>>
>> Also, if we decide, that this all is too huge, here is plan B:
>>
>> 1. apply 01
>> 2. fix only functions that don't use local_err and use
>> error_append_hint, by just invocation of new macro at function start -
>> it will substitute Greg's series with no pain.
>> 3[optional]. Do full update for some subsystems, for example, only for
>> block* and nbd*
> 
> Even if we go with plan B, it's still worth checking in a Coccinelle
> script that we can periodically run to make sure we aren't missing out
> on the use of the macro where it is needed.
> 
>>
>> Vladimir Sementsov-Ogievskiy (9):
>>    error: auto propagated local_err
>>    qapi/error: add (Error **errp) cleaning APIs
>>    errp: rename errp to errp_in where it is IN-argument
>>    hw/core/loader-fit: fix freeing errp in fit_load_fdt
>>    net/net: fix local variable shadowing in net_client_init
>>    scripts: add coccinelle script to use auto propagated errp
>>    Use auto-propagated errp
>>    fix-compilation: empty goto
>>    fix-compilation: includes
>>
>>   include/hw/pci-host/spapr.h                   |   2 +
>>   include/monitor/hmp.h                         |   2 +-
>>   include/qapi/error.h                          |  61 ++++-
>>   target/ppc/kvm_ppc.h                          |   3 +
>>   target/s390x/cpu_models.h                     |   3 +
>>   ui/vnc.h                                      |   2 +-
> 
>>   vl.c                                          |  13 +-
>>   scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>   319 files changed, 2729 insertions(+), 4245 deletions(-)
>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
> 
> The diffstat is huge, but promising.
> 


-- 
Best regards,
Vladimir

Re: [RFC v2 0/9] error: auto propagated local_err

[Eric Blake]

On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> 3. What to do with huge auto-generated commit 07? Should I split it
>>> per-maintainer or per-subsystem, or leave it as-is?
>>
>> It's big. I'd split it into multiple patches (and reduce the cc - except
>> for the cover letter, the rest of the patches can be limited to the
>> actual maintainer/subsystem affected rather than everyone involved
>> anywhere else in the series. With the current large cc, anyone that
>> replies gets several mail bounces about "too many recipients").  It may
>> be easier to split along directory boundaries than by maintainer
>> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
>> before, maybe he has some advice.
> 
> 
> If split by subsystem it would be 200+ patches:
> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
> 205
> 
> 
> Try to look at larger subsystem:
> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
> 139
> 
> still too many.. Or is it OK?

Hmm - that becomes a tradeoff in length of the series (where individual
patches may be reviewed fast, but where the overall process may be
bogged down by sheer length), vs. length of individual emails (where the
email itself is daunting, but as the review is mechanical and done by
automation, it becomes a matter of spot-checking if we trust that the
automation was done correctly).  You can probably group it in fewer
patches, by joining smaller patches across a couple of subsystems.  It's
an art form, there's probably several ways to do it that would work, and
it comes down to a judgment call on how much work you want to do to try
and reduce other's work in reviewing it.  Maybe even an off-hand split
of gathering files until you reach about 500 or so lines per diff.  I
wish I had easier advice on how to tackle this sort of project in the
way that will get the fastest response time.


>>>   vl.c                                          |  13 +-
>>>   scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>>   319 files changed, 2729 insertions(+), 4245 deletions(-)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>
>> The diffstat is huge, but promising.

We also learned in reviews of 7/9 that the diffstat here is misleading,
the number of insertions will definitely be increasing once the
Coccinelle script is fixed to insert the macro in more functions, but
hopefully it's still a net reduction in overall lines.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 0/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

24.09.2019 18:28, Eric Blake wrote:
> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>> 3. What to do with huge auto-generated commit 07? Should I split it
>>>> per-maintainer or per-subsystem, or leave it as-is?
>>>
>>> It's big. I'd split it into multiple patches (and reduce the cc - except
>>> for the cover letter, the rest of the patches can be limited to the
>>> actual maintainer/subsystem affected rather than everyone involved
>>> anywhere else in the series. With the current large cc, anyone that
>>> replies gets several mail bounces about "too many recipients").  It may
>>> be easier to split along directory boundaries than by maintainer
>>> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
>>> before, maybe he has some advice.
>>
>>
>> If split by subsystem it would be 200+ patches:
>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
>> 205
>>
>>
>> Try to look at larger subsystem:
>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
>> 139
>>
>> still too many.. Or is it OK?
> 
> Hmm - that becomes a tradeoff in length of the series (where individual
> patches may be reviewed fast, but where the overall process may be
> bogged down by sheer length), vs. length of individual emails (where the
> email itself is daunting, but as the review is mechanical and done by
> automation, it becomes a matter of spot-checking if we trust that the
> automation was done correctly).  You can probably group it in fewer
> patches, by joining smaller patches across a couple of subsystems.  It's
> an art form, there's probably several ways to do it that would work, and
> it comes down to a judgment call on how much work you want to do to try
> and reduce other's work in reviewing it.  Maybe even an off-hand split
> of gathering files until you reach about 500 or so lines per diff.  I
> wish I had easier advice on how to tackle this sort of project in the
> way that will get the fastest response time.
> 
> 
>>>>    vl.c                                          |  13 +-
>>>>    scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>>>    319 files changed, 2729 insertions(+), 4245 deletions(-)
>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> The diffstat is huge, but promising.
> 
> We also learned in reviews of 7/9 that the diffstat here is misleading,
> the number of insertions will definitely be increasing once the
> Coccinelle script is fixed to insert the macro in more functions, but
> hopefully it's still a net reduction in overall lines.
> 

No hope for us: with fixed script I now see

919 files changed, 6425 insertions(+), 4234 deletions(-)

-- 
Best regards,
Vladimir

Re: [RFC v2 0/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

24.09.2019 18:44, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2019 18:28, Eric Blake wrote:
>> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>>> 3. What to do with huge auto-generated commit 07? Should I split it
>>>>> per-maintainer or per-subsystem, or leave it as-is?
>>>>
>>>> It's big. I'd split it into multiple patches (and reduce the cc - except
>>>> for the cover letter, the rest of the patches can be limited to the
>>>> actual maintainer/subsystem affected rather than everyone involved
>>>> anywhere else in the series. With the current large cc, anyone that
>>>> replies gets several mail bounces about "too many recipients").  It may
>>>> be easier to split along directory boundaries than by maintainer
>>>> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
>>>> before, maybe he has some advice.
>>>
>>>
>>> If split by subsystem it would be 200+ patches:
>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
>>> 205
>>>
>>>
>>> Try to look at larger subsystem:
>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
>>> 139
>>>
>>> still too many.. Or is it OK?
>>
>> Hmm - that becomes a tradeoff in length of the series (where individual
>> patches may be reviewed fast, but where the overall process may be
>> bogged down by sheer length), vs. length of individual emails (where the
>> email itself is daunting, but as the review is mechanical and done by
>> automation, it becomes a matter of spot-checking if we trust that the
>> automation was done correctly).  You can probably group it in fewer
>> patches, by joining smaller patches across a couple of subsystems.  It's
>> an art form, there's probably several ways to do it that would work, and
>> it comes down to a judgment call on how much work you want to do to try
>> and reduce other's work in reviewing it.  Maybe even an off-hand split
>> of gathering files until you reach about 500 or so lines per diff.  I
>> wish I had easier advice on how to tackle this sort of project in the
>> way that will get the fastest response time.
>>
>>
>>>>>    vl.c                                          |  13 +-
>>>>>    scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>>>>    319 files changed, 2729 insertions(+), 4245 deletions(-)
>>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>
>>>> The diffstat is huge, but promising.
>>
>> We also learned in reviews of 7/9 that the diffstat here is misleading,
>> the number of insertions will definitely be increasing once the
>> Coccinelle script is fixed to insert the macro in more functions, but
>> hopefully it's still a net reduction in overall lines.
>>
> 
> No hope for us: with fixed script I now see
> 
> 919 files changed, 6425 insertions(+), 4234 deletions(-)
> 

Also, I have add include "qapi/error.h" to files, where errp only passed
to called functions (or for functions, which are not simple stubs):

# git diff | grep '+#include' | wc -l
253


-- 
Best regards,
Vladimir

Re: [RFC v2 0/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

24.09.2019 18:46, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2019 18:44, Vladimir Sementsov-Ogievskiy wrote:
>> 24.09.2019 18:28, Eric Blake wrote:
>>> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>>>> 3. What to do with huge auto-generated commit 07? Should I split it
>>>>>> per-maintainer or per-subsystem, or leave it as-is?
>>>>>
>>>>> It's big. I'd split it into multiple patches (and reduce the cc - except
>>>>> for the cover letter, the rest of the patches can be limited to the
>>>>> actual maintainer/subsystem affected rather than everyone involved
>>>>> anywhere else in the series. With the current large cc, anyone that
>>>>> replies gets several mail bounces about "too many recipients").  It may
>>>>> be easier to split along directory boundaries than by maintainer
>>>>> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
>>>>> before, maybe he has some advice.
>>>>
>>>>
>>>> If split by subsystem it would be 200+ patches:
>>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
>>>> 205
>>>>
>>>>
>>>> Try to look at larger subsystem:
>>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
>>>> 139
>>>>
>>>> still too many.. Or is it OK?
>>>
>>> Hmm - that becomes a tradeoff in length of the series (where individual
>>> patches may be reviewed fast, but where the overall process may be
>>> bogged down by sheer length), vs. length of individual emails (where the
>>> email itself is daunting, but as the review is mechanical and done by
>>> automation, it becomes a matter of spot-checking if we trust that the
>>> automation was done correctly).  You can probably group it in fewer
>>> patches, by joining smaller patches across a couple of subsystems.  It's
>>> an art form, there's probably several ways to do it that would work, and
>>> it comes down to a judgment call on how much work you want to do to try
>>> and reduce other's work in reviewing it.  Maybe even an off-hand split
>>> of gathering files until you reach about 500 or so lines per diff.  I
>>> wish I had easier advice on how to tackle this sort of project in the
>>> way that will get the fastest response time.

git diff | wc -l
48941

so, by 500 lines it would be 97 patches.

Seems, we'll never be able to review this thing :(

Any ideas?

May be, separate big patch, which just add ERRP_FUNCTION_BEGIN() to all errp functions?

>>>
>>>
>>>>>>    vl.c                                          |  13 +-
>>>>>>    scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>>>>>    319 files changed, 2729 insertions(+), 4245 deletions(-)
>>>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>>
>>>>> The diffstat is huge, but promising.
>>>
>>> We also learned in reviews of 7/9 that the diffstat here is misleading,
>>> the number of insertions will definitely be increasing once the
>>> Coccinelle script is fixed to insert the macro in more functions, but
>>> hopefully it's still a net reduction in overall lines.
>>>
>>
>> No hope for us: with fixed script I now see
>>
>> 919 files changed, 6425 insertions(+), 4234 deletions(-)
>>
> 
> Also, I have add include "qapi/error.h" to files, where errp only passed
> to called functions (or for functions, which are not simple stubs):
> 
> # git diff | grep '+#include' | wc -l
> 253
> 
> 


-- 
Best regards,
Vladimir

Re: [RFC v2 0/9] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

24.09.2019 18:49, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2019 18:46, Vladimir Sementsov-Ogievskiy wrote:
>> 24.09.2019 18:44, Vladimir Sementsov-Ogievskiy wrote:
>>> 24.09.2019 18:28, Eric Blake wrote:
>>>> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>
>>>>>>> 3. What to do with huge auto-generated commit 07? Should I split it
>>>>>>> per-maintainer or per-subsystem, or leave it as-is?
>>>>>>
>>>>>> It's big. I'd split it into multiple patches (and reduce the cc - except
>>>>>> for the cover letter, the rest of the patches can be limited to the
>>>>>> actual maintainer/subsystem affected rather than everyone involved
>>>>>> anywhere else in the series. With the current large cc, anyone that
>>>>>> replies gets several mail bounces about "too many recipients").  It may
>>>>>> be easier to split along directory boundaries than by maintainer
>>>>>> boundaries.  Markus has applied large tree-wide Coccinelle cleanups
>>>>>> before, maybe he has some advice.
>>>>>
>>>>>
>>>>> If split by subsystem it would be 200+ patches:
>>>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l
>>>>> 205
>>>>>
>>>>>
>>>>> Try to look at larger subsystem:
>>>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
>>>>> 139
>>>>>
>>>>> still too many.. Or is it OK?
>>>>
>>>> Hmm - that becomes a tradeoff in length of the series (where individual
>>>> patches may be reviewed fast, but where the overall process may be
>>>> bogged down by sheer length), vs. length of individual emails (where the
>>>> email itself is daunting, but as the review is mechanical and done by
>>>> automation, it becomes a matter of spot-checking if we trust that the
>>>> automation was done correctly).  You can probably group it in fewer
>>>> patches, by joining smaller patches across a couple of subsystems.  It's
>>>> an art form, there's probably several ways to do it that would work, and
>>>> it comes down to a judgment call on how much work you want to do to try
>>>> and reduce other's work in reviewing it.  Maybe even an off-hand split
>>>> of gathering files until you reach about 500 or so lines per diff.  I
>>>> wish I had easier advice on how to tackle this sort of project in the
>>>> way that will get the fastest response time.
> 
> git diff | wc -l
> 48941
> 
> so, by 500 lines it would be 97 patches.
> 
> Seems, we'll never be able to review this thing :(
> 
> Any ideas?
> 
> May be, separate big patch, which just add ERRP_FUNCTION_BEGIN() to all errp functions?

checked: for remaining improvements:
git diff | wc -l
20218

git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l
90

Still, too much..

I think we should switch to plan B, at least as something that may be merged to 4.2


> 
>>>>
>>>>
>>>>>>>    vl.c                                          |  13 +-
>>>>>>>    scripts/coccinelle/auto-propagated-errp.cocci |  82 +++++++
>>>>>>>    319 files changed, 2729 insertions(+), 4245 deletions(-)
>>>>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>>>
>>>>>> The diffstat is huge, but promising.
>>>>
>>>> We also learned in reviews of 7/9 that the diffstat here is misleading,
>>>> the number of insertions will definitely be increasing once the
>>>> Coccinelle script is fixed to insert the macro in more functions, but
>>>> hopefully it's still a net reduction in overall lines.
>>>>
>>>
>>> No hope for us: with fixed script I now see
>>>
>>> 919 files changed, 6425 insertions(+), 4234 deletions(-)
>>>
>>
>> Also, I have add include "qapi/error.h" to files, where errp only passed
>> to called functions (or for functions, which are not simple stubs):
>>
>> # git diff | grep '+#include' | wc -l
>> 253
>>
>>
> 
> 


-- 
Best regards,
Vladimir