[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friend
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends |
|
Date: |
Thu, 25 Jun 2020 22:00:36 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 |
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<armbru@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