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.
> 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
In my experience, I need to correct Coccinelle's spacing more often than
not. I usually don't bother to enumerate my corrections one by one, I
only document what kind of corrections I made.
