24.06.2020 19:43, Markus Armbruster wrote:
Convert

     foo(..., &err);
     if (err) {
         ...
     }

to

     if (!foo(..., &err)) {
         ...
     }

for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their
wrappers isa_realize_and_unref(), pci_realize_and_unref(),
sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref().
Coccinelle script:

Please, also mention a command to run the script


     @@
     identifier fun = {isa_realize_and_unref, pci_realize_and_unref, 
qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, 
sysbus_realize_and_unref, usb_realize_and_unref};
     expression list args, args2;
     typedef Error;
     Error *err;
     identifier errp;
     @@
     -      fun(args, &err, args2);
     -      if (err) {
     +      if (!fun(args, errp, args2)) {
               ... when != err
     -     error_propagate(errp, err);
               ...
           }

     @@
     identifier fun = {isa_realize_and_unref, pci_realize_and_unref, 
qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, 
sysbus_realize_and_unref, usb_realize_and_unref};
     expression list args, args2;
     typedef Error;
     Error *err;
     @@
     -      fun(args, &err, args2);
     -      if (err) {
     +      if (!fun(args, &err, args2)) {
               ...
           }

Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
ARMSSE being used both as typedef and function-like macro there.
Convert manually.

Eliminate error_propagate() that are now unnecessary.  Delete @err
that are now unused.  Clean up whitespace.

Signed-off-by: Markus Armbruster<arm...@redhat.com>
---
  hw/arm/allwinner-a10.c          |  21 ++-----
  hw/arm/armsse.c                 | 104 ++++++++------------------------
  hw/arm/armv7m.c                 |  12 +---
  hw/arm/aspeed_ast2600.c         |  68 ++++++---------------
  hw/arm/aspeed_soc.c             |  60 +++++-------------
  hw/arm/bcm2835_peripherals.c    |  60 +++++-------------
  hw/arm/bcm2836.c                |  12 +---
  hw/arm/cubieboard.c             |   3 +-
  hw/arm/digic.c                  |  12 +---
  hw/arm/digic_boards.c           |   3 +-
  hw/arm/fsl-imx25.c              |  44 ++++----------
  hw/arm/fsl-imx31.c              |  32 +++-------
  hw/arm/fsl-imx6.c               |  48 ++++-----------
  hw/arm/msf2-soc.c               |  21 ++-----
  hw/arm/nrf51_soc.c              |  24 ++------
  hw/arm/stm32f205_soc.c          |  29 +++------
  hw/arm/stm32f405_soc.c          |  32 +++-------
  hw/arm/xlnx-zynqmp.c            |  61 +++++--------------
  hw/block/fdc.c                  |   4 +-
  hw/block/xen-block.c            |   3 +-
  hw/char/serial-pci-multi.c      |   5 +-
  hw/char/serial-pci.c            |   5 +-
  hw/char/serial.c                |  10 +--
  hw/core/cpu.c                   |   3 +-
  hw/cpu/a15mpcore.c              |   5 +-
  hw/cpu/a9mpcore.c               |  21 ++-----
  hw/cpu/arm11mpcore.c            |  17 ++----
  hw/cpu/realview_mpcore.c        |   9 +--
  hw/display/virtio-gpu-pci.c     |   6 +-
  hw/display/virtio-vga.c         |   5 +-
  hw/intc/armv7m_nvic.c           |   9 +--
  hw/intc/pnv_xive.c              |   8 +--
  hw/intc/realview_gic.c          |   5 +-
  hw/intc/spapr_xive.c            |   8 +--
  hw/intc/xics.c                  |   5 +-
  hw/intc/xive.c                  |   3 +-
  hw/isa/piix4.c                  |   5 +-
  hw/microblaze/xlnx-zynqmp-pmu.c |   9 +--
  hw/mips/cps.c                   |  17 ++----
  hw/misc/macio/cuda.c            |   5 +-
  hw/misc/macio/macio.c           |  25 ++------
  hw/misc/macio/pmu.c             |   5 +-
  hw/pci-host/pnv_phb3.c          |  13 +---
  hw/pci-host/pnv_phb4.c          |   5 +-
  hw/pci-host/pnv_phb4_pec.c      |   5 +-
  hw/ppc/e500.c                   |   5 +-
  hw/ppc/pnv.c                    |  53 ++++------------
  hw/ppc/pnv_core.c               |   4 +-
  hw/ppc/pnv_psi.c                |   9 +--
  hw/ppc/spapr_cpu_core.c         |   3 +-
  hw/ppc/spapr_irq.c              |   5 +-
  hw/riscv/opentitan.c            |   9 +--
  hw/riscv/sifive_e.c             |   6 +-
  hw/riscv/sifive_u.c             |   5 +-
  hw/s390x/event-facility.c       |  13 ++--
  hw/s390x/s390-pci-bus.c         |   3 +-
  hw/s390x/sclp.c                 |   3 +-
  hw/s390x/virtio-ccw-crypto.c    |   5 +-
  hw/s390x/virtio-ccw-rng.c       |   5 +-
  hw/scsi/scsi-bus.c              |   4 +-
  hw/sd/aspeed_sdhci.c            |   4 +-
  hw/sd/ssi-sd.c                  |   3 +-
  hw/usb/bus.c                    |   3 +-
  hw/virtio/virtio-rng-pci.c      |   5 +-
  qdev-monitor.c                  |   3 +-
 65 files changed, 248 insertions(+), 768 deletions(-)

Almost all of this diff-stat may be generated by more obvious script:

@rule1@
identifier fun = {qdev_realize, qdev_realize_and_unref, sysbus_realize};
expression list args;
typedef Error;
Error *err;
identifier errp;
@@

-      fun(args, &err);
-      if (err)
+      if (!fun(args, errp))
       {
-              error_propagate(errp, err);
           return;
       }

@depends on rule1@
identifier err;
@@

-    Error *err = NULL;
     ... when != err


===
Script started by command
spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h --in-place 
--no-show-diff --max-width 80 --use-gitgrep hw

You see, I consider only obvious case, where we have only error_propagate + 
return in the if. I suggest to make a separate generated patch, based on my 
cocci script (it's mostly yours, actually), and as a second patch - the 
remaining of your patch. I do think that this will simplify the review.

diffstat generated by my cocci script:
 50 files changed, 230 insertions(+), 725 deletions(-)

Difference with your patch, considering only files, touched by new script is 
very small:

[root@kvm error-smooth-my-try]# git diff --name-only | xargs git diff 
bbdd4596d3e3290 --
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 416adfcce0..cacfcbac54 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -185,6 +185,7 @@ static void sifive_e_soc_realize(DeviceState *dev, Error 
**errp)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
     const struct MemmapEntry *memmap = sifive_e_memmap;
+
     SiFiveESoCState *s = RISCV_E_SOC(dev);
     MemoryRegion *sys_mem = get_system_memory();
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 645b4080c5..32ff015c2c 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -442,13 +442,15 @@ static void init_event_facility(Object *obj)
 static void realize_event_facility(DeviceState *dev, Error **errp)
 {
     SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
+    Error *local_err = NULL;
- if (!qdev_realize(DEVICE(&event_facility->quiesce),
-                      BUS(&event_facility->sbus), errp)) {
+    if (!qdev_realize(DEVICE(&event_facility->quiesce), 
BUS(&event_facility->sbus), errp)) {
         return;
     }
-    if (!qdev_realize(DEVICE(&event_facility->cpu_hotplug),
-                      BUS(&event_facility->sbus), errp)) {
+    qdev_realize(DEVICE(&event_facility->cpu_hotplug),
+                 BUS(&event_facility->sbus), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         qdev_unrealize(DEVICE(&event_facility->quiesce));
         return;
     }



- Hmm, strange, "--max-width 80" doesn't help. So, we may keep the patch 
fully-generated with only one over-80 line, or write in commit message that the only 
manual fix is one over-80 line wrapping in hw/s390x/event-facility.c


--
Best regards,
Vladimir

Reply via email to