On Mon, 06 Jul 2020 13:35:19 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Greg Kurz <gr...@kaod.org> writes:
>
> > On Mon, 6 Jul 2020 10:09:09 +0200
> > Markus Armbruster <arm...@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 <arm...@redhat.com>
> >> Reviewed-by: Eric Blake <ebl...@redhat.com>
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> >> ---
> >
> > FWIW I had posted an R-b for this patch in v1
> > (20200629124037.2b9a2...@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.
