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

From: Markus Armbruster <armbru@redhat.com>
To: Greg Kurz <groug@kaod.org>
Cc: peter.maydell@linaro.org, vsementsov@virtuozzo.com,
	berrange@redhat.com, ehabkost@redhat.com, qemu-block@nongnu.org,
	qemu-devel@nongnu.org, pbonzini@redhat.com
Subject: Re: [PATCH v3 03/44] qdev: Use returned bool to check for qdev_realize() etc. failure
Date: Mon, 06 Jul 2020 13:35:19 +0200	[thread overview]
Message-ID: <87k0zganbc.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200706120718.70c37bc0@bahia.lan> (Greg Kurz's message of "Mon,  6 Jul 2020 12:08:16 +0200")

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?