Re: [PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure

2020-07-02 Thread Vladimir Sementsov-Ogievskiy 

02.07.2020 18:49, Markus Armbruster wrote:

Convert

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

to

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

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:

 @@
 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, , args2);
 -if (err)
 +if (!fun(args, , args2))
  {
  ...
  }

Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error
message "no position information".  Nothing to convert there; skipped.

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

A few line breaks tidied up manually.

Signed-off-by: Markus Armbruster



Sorry me, reviewing this patch with help of script:
#!/usr/bin/env python3

import sys
import re

with open(sys.argv[1]) as f:
patch = f.read()

regex = re.compile(r'^- *(?P(?P\w+)\(.*, &(?P\w+)\));\n'
   r'^- *if \((?P=err)( != NULL)?\) \{\n'
   r'^\+ *if \(!(?P=func_call)\) \{$', flags=re.MULTILINE)

for chunk in re.split('^@', patch, flags=re.MULTILINE):
filtered = regex.sub('OK', chunk)

if re.search('^[+-][^+-]', filtered, flags=re.MULTILINE):
print(re.sub('^', '   ', '@' + chunk, flags=re.MULTILINE))


funcs = set()

for m in regex.finditer(patch):
funcs.add(m.group('func'))

print()
for func in funcs:
print(func)



output:

   @@ -34,9 +34,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
Error *local_error = NULL;

virtio_pci_force_virtio_1(vpci_dev);

   -qdev_realize(vdev, BUS(_dev->bus), _error);
   -
   -if (local_error) {
   +if (!qdev_realize(vdev, BUS(_dev->bus), _error)) {
error_propagate(errp, local_error);
return;
}
   diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
   index 67f409e106..0fc00fee1f 100644
   --- a/hw/display/virtio-vga.c
   +++ b/hw/display/virtio-vga.c
   
   @@ -444,15 +444,13 @@ static void realize_event_facility(DeviceState *dev, Error **errp)

SCLPEventFacility *event_facility = EVENT_FACILITY(dev);
Error *local_err = NULL;

   -qdev_realize(DEVICE(_facility->quiesce),

   - BUS(_facility->sbus), _err);
   -if (local_err) {
   +if (!qdev_realize(DEVICE(_facility->quiesce),
   +  BUS(_facility->sbus), _err)) {
error_propagate(errp, local_err);
return;
}
   -qdev_realize(DEVICE(_facility->cpu_hotplug),
   - BUS(_facility->sbus), _err);
   -if (local_err) {
   +if (!qdev_realize(DEVICE(_facility->cpu_hotplug),
   +  BUS(_facility->sbus), _err)) {
error_propagate(errp, local_err);
qdev_unrealize(DEVICE(_facility->quiesce));
return;
   diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
   index 142e52a8ff..0517901024 100644
   --- a/hw/s390x/s390-pci-bus.c
   +++ b/hw/s390x/s390-pci-bus.c
   


usb_realize_and_unref
sysbus_realize
sysbus_realize_and_unref
qdev_realize
qdev_realize_and_unref

===

So, the remaning non-matching seems correct, and all found functions seems to 
have corresponding semantics:

Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure

2020-07-02 Thread Eric Blake 

On 7/2/20 10:49 AM, Markus Armbruster wrote:

Convert

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

to

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

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:

 @@
 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, , args2);
 -if (err)
 +if (!fun(args, , args2))
  {
  ...
  }

Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error
message "no position information".  Nothing to convert there; skipped.

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

A few line breaks tidied up manually.

Signed-off-by: Markus Armbruster 
---


The conversion is quite straight-forward.  The patch is big, but 
correct, and you documented where it is not mechanical.


Reviewed-by: Eric Blake 

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

[PATCH v2 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure

2020-07-02 Thread Markus Armbruster 
Convert

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

to

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

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:

@@
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, , args2);
-if (err)
+if (!fun(args, , args2))
 {
 ...
 }

Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error
message "no position information".  Nothing to convert there; skipped.

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

A few line breaks tidied up manually.

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

diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index 52e0d83760..e1acffe5f6 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -74,14 +74,12 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
 SysBusDevice *sysbusdev;
 Error *err = NULL;
 
-qdev_realize(DEVICE(>cpu), NULL, );
-if (err != NULL) {
+if (!qdev_realize(DEVICE(>cpu), NULL, )) {
 error_propagate(errp, err);
 return;
 }
 
-sysbus_realize(SYS_BUS_DEVICE(>intc), );
-if (err != NULL) {
+if (!sysbus_realize(SYS_BUS_DEVICE(>intc), )) {
 error_propagate(errp, err);
 return;
 }
@@ -93,8 +91,7 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
qdev_get_gpio_in(DEVICE(>cpu), ARM_CPU_FIQ));
 qdev_pass_gpios(DEVICE(>intc), dev, NULL);
 
-sysbus_realize(SYS_BUS_DEVICE(>timer), );
-if (err != NULL) {
+if (!sysbus_realize(SYS_BUS_DEVICE(>timer), ))