qemu-block
[Top][All Lists]
Advanced

[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: Markus Armbruster
Subject: Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends
Date: Fri, 26 Jun 2020 08:19:29 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

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.

> 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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]