Markus Armbruster <arm...@redhat.com> writes:
> Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
>
>> 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.
>
> I can try this idea. It's not just this patch, though, it's four more:
> PATCH 17+23+38+42.
I did this instead for v2:
* Use a trivial, safe script for converting to use the returned bool to
check for failure.
65 files changed, 248 insertions(+), 495 deletions(-)
32 files changed, 71 insertions(+), 132 deletions(-)
46 files changed, 97 insertions(+), 188 deletions(-)
28 files changed, 96 insertions(+), 142 deletions(-)
3 files changed, 4 insertions(+), 7 deletions(-)
* Use a an admittedly more complex script for eliminating many
error_propagate(). I consider the script safe. I believe it's
reasonably easy to understand.
114 files changed, 376 insertions(+), 884 deletions(-)
* Use an unsafe variant of the same script for eliminating a few more:
23 files changed, 32 insertions(+), 78 deletions(-)
Even though rather I like how it came out, it may have been a bad idea,
because time's awfully short now.
