Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Vladimir Sementsov-Ogievskiy <vsementsov@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<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.
>
> 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.