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

[Greg Kurz]

On Mon, 06 Jul 2020 13:35:19 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > On Mon,  6 Jul 2020 10:09:09 +0200
> > Markus Armbruster <armbru@redhat.com> 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:
> >> 
> >>     @@
> >>     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))
> >>          {
> >>              ...
> >>          }
> >> 
> >> 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 <armbru@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >
> > FWIW I had posted an R-b for this patch in v1 
> > (20200629124037.2b9a269e@bahia.lan).
> 
> When I sliced and diced my patches for v2, I dropped R-bys for patches
> substantially altered.  This one was borderline: the patch does strictly
> less, and the work it no longer does us done by later patches.
> 
> Example: v1's first hunk
> 
>     diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
>     index 52e0d83760..3e45aa4141 100644
>     --- a/hw/arm/allwinner-a10.c
>     +++ b/hw/arm/allwinner-a10.c
>     @@ -72,17 +72,12 @@ static void aw_a10_realize(DeviceState *dev, Error 
> **errp)
>      {
>          AwA10State *s = AW_A10(dev);
>          SysBusDevice *sysbusdev;
>     -    Error *err = NULL;
> 
>     -    qdev_realize(DEVICE(&s->cpu), NULL, &err);
>     -    if (err != NULL) {
>     -        error_propagate(errp, err);
>     +    if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
>              return;
>          }
> 
>     -    sysbus_realize(SYS_BUS_DEVICE(&s->intc), &err);
>     -    if (err != NULL) {
>     -        error_propagate(errp, err);
>     +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->intc), errp)) {
>              return;
>          }
>          sysbusdev = SYS_BUS_DEVICE(&s->intc);
> 
> became
> 
>     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(&s->cpu), NULL, &err);
>     -    if (err != NULL) {
>     +    if (!qdev_realize(DEVICE(&s->cpu), NULL, &err)) {
>              error_propagate(errp, err);
>              return;
>          }
> 
>     -    sysbus_realize(SYS_BUS_DEVICE(&s->intc), &err);
>     -    if (err != NULL) {
>     +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->intc), &err)) {
>              error_propagate(errp, err);
>              return;
>          }
> 
> 
> in v2 and v3.  The two error_propagate() and the local variable now go
> away only in PATCH v3 33.
> 
> Would you like me to record your R-by for the patch's current version?
> 

I've reviewed it again, so, yes, please do.