From: Alistair Francis <alistair23@gmail.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>,
Alistair Francis <alistair@alistair23.me>,
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
Eric Auger <eric.auger@redhat.com>,
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
qemu-arm <qemu-arm@nongnu.org>, Gerd Hoffmann <kraxel@redhat.com>,
"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH 16/22] hw: Fix error API violation around object_property_set_link()
Date: Mon, 22 Jun 2020 14:22:13 -0700 [thread overview]
Message-ID: <CAKmqyKM2bUbgZD7gEFJ9A99dQ2zzCoSydOrp51pEeWmWCpE+bA@mail.gmail.com> (raw)
In-Reply-To: <20200622104250.1404835-17-armbru@redhat.com>
On Mon, Jun 22, 2020 at 3:53 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL. Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
>
> virtio_gpu_pci_base_realize(), virtio_vga_base_realize(),
> sparc32_ledma_device_realize(), sparc32_dma_realize(),
> sparc32_dma_realize() xilinx_axidma_realize(), mips_cps_realize(),
> macio_realize_ide(), xilinx_enet_realize(), and
> virtio_iommu_pci_realize() are wrong that way: they reuse the argument
> they pass to object_property_set_link() for another call.
>
> Harmless, because object_property_set_link() can't actually fail for
> them: it fails when the property doesn't exist, is not settable, or
> its .check() method fails. Fix by passing &error_abort instead.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/display/virtio-gpu-pci.c | 2 +-
> hw/display/virtio-vga.c | 2 +-
> hw/dma/sparc32_dma.c | 6 +++---
> hw/dma/xilinx_axidma.c | 12 ++----------
> hw/mips/cps.c | 6 ++++--
> hw/misc/macio/macio.c | 3 ++-
> hw/net/xilinx_axienet.c | 12 ++----------
> hw/virtio/virtio-iommu-pci.c | 2 +-
> 8 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
> index b532fe8b5f..41b88b878d 100644
> --- a/hw/display/virtio-gpu-pci.c
> +++ b/hw/display/virtio-gpu-pci.c
> @@ -44,7 +44,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> for (i = 0; i < g->conf.max_outputs; i++) {
> object_property_set_link(OBJECT(g->scanout[i].con),
> OBJECT(vpci_dev),
> - "device", errp);
> + "device", &error_abort);
> }
> }
>
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index 68a062ece6..67f409e106 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -154,7 +154,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> for (i = 0; i < g->conf.max_outputs; i++) {
> object_property_set_link(OBJECT(g->scanout[i].con),
> OBJECT(vpci_dev),
> - "device", errp);
> + "device", &error_abort);
> }
> }
>
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index f02aca6f40..2d7dbbb92d 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
> d = qdev_new(TYPE_LANCE);
> object_property_add_child(OBJECT(dev), "lance", OBJECT(d));
> qdev_set_nic_properties(d, nd);
> - object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp);
> + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", &error_abort);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
> }
>
> @@ -379,7 +379,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
> }
>
> espdma = qdev_new(TYPE_SPARC32_ESPDMA_DEVICE);
> - object_property_set_link(OBJECT(espdma), iommu, "iommu", errp);
> + object_property_set_link(OBJECT(espdma), iommu, "iommu", &error_abort);
> object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(espdma), &error_fatal);
>
> @@ -394,7 +394,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
> sysbus_mmio_get_region(sbd, 0));
>
> ledma = qdev_new(TYPE_SPARC32_LEDMA_DEVICE);
> - object_property_set_link(OBJECT(ledma), iommu, "iommu", errp);
> + object_property_set_link(OBJECT(ledma), iommu, "iommu", &error_abort);
> object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(ledma), &error_fatal);
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 6a9df2c4db..a069637bf2 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -537,7 +537,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
> XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev);
> XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(
> &s->rx_control_dev);
> - Error *local_err = NULL;
> int i;
>
> object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
> @@ -548,11 +547,8 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
> (Object **)&cs->dma,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG);
> - object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &local_err);
> - object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &local_err);
> - if (local_err) {
> - goto xilinx_axidma_realize_fail;
> - }
> + object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &error_abort);
> + object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &error_abort);
>
> for (i = 0; i < 2; i++) {
> struct Stream *st = &s->streams[i];
> @@ -567,10 +563,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
>
> address_space_init(&s->as,
> s->dma_mr ? s->dma_mr : get_system_memory(), "dma");
> - return;
> -
> -xilinx_axidma_realize_fail:
> - error_propagate(errp, local_err);
> }
>
> static void xilinx_axidma_init(Object *obj)
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index cdfab19826..5382bc86f7 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -150,8 +150,10 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
> object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp", &err);
> object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev", &err);
> object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base", &err);
> - object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic", &err);
> - object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc", &err);
> + object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic",
> + &error_abort);
> + object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc",
> + &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
> if (err != NULL) {
> error_propagate(errp, err);
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 8ba7af073c..3251c79f46 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -136,7 +136,8 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
> sysbus_connect_irq(sysbus_dev, 0, irq0);
> sysbus_connect_irq(sysbus_dev, 1, irq1);
> qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
> - object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma", errp);
> + object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma",
> + &error_abort);
> macio_ide_register_dma(ide);
>
> qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index c2f40b8ea9..679a359f9a 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -980,7 +980,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
> XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev);
> XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(
> &s->rx_control_dev);
> - Error *local_err = NULL;
>
> object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
> (Object **) &ds->enet,
> @@ -990,11 +989,8 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
> (Object **) &cs->enet,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG);
> - object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &local_err);
> - object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &local_err);
> - if (local_err) {
> - goto xilinx_enet_realize_fail;
> - }
> + object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &error_abort);
> + object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &error_abort);
>
> qemu_macaddr_default_if_unset(&s->conf.macaddr);
> s->nic = qemu_new_nic(&net_xilinx_enet_info, &s->conf,
> @@ -1008,10 +1004,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
>
> s->rxmem = g_malloc(s->c_rxmem);
> s->txmem = g_malloc(s->c_txmem);
> - return;
> -
> -xilinx_enet_realize_fail:
> - error_propagate(errp, local_err);
> }
>
> static void xilinx_enet_init(Object *obj)
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index 632533abaf..bd61d6e2f8 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -56,7 +56,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> }
> object_property_set_link(OBJECT(dev),
> OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
> - "primary-bus", errp);
> + "primary-bus", &error_abort);
> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> }
>
> --
> 2.26.2
>
>
next prev parent reply other threads:[~2020-06-22 21:33 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
2020-06-22 10:42 ` [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression Markus Armbruster
2020-06-22 21:25 ` Jens Freimann
2020-06-24 15:12 ` Michael S. Tsirkin
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
2020-06-22 21:26 ` Jens Freimann
2020-06-24 15:13 ` Michael S. Tsirkin
2020-06-22 10:42 ` [PATCH 03/22] Clean up some calls to ignore Error objects the right way Markus Armbruster
2020-06-22 14:43 ` Greg Kurz
2020-06-22 10:42 ` [PATCH 04/22] tests: Use &error_abort where appropriate Markus Armbruster
2020-06-22 11:26 ` Thomas Huth
2020-06-22 10:42 ` [PATCH 05/22] tests: Use error_free_or_abort() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 06/22] usb/dev-mtp: Fix Error double free after inotify failure Markus Armbruster
2020-06-22 10:42 ` [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init() Markus Armbruster
2020-06-22 14:54 ` Greg Kurz
2020-06-22 10:42 ` [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks Markus Armbruster
2020-06-23 8:35 ` Zhanghailiang
2020-06-22 10:42 ` [PATCH 09/22] sd/milkymist-memcard: Plug minor memory leak in realize Markus Armbruster
2020-06-22 10:42 ` [PATCH 10/22] test-util-filemonitor: Plug unlikely memory leak Markus Armbruster
2020-06-22 10:42 ` [PATCH 11/22] vnc: Plug minor memory leak in vnc_display_open() Markus Armbruster
2020-06-22 10:42 ` [PATCH 12/22] tests/qom-proplist: Delete a superfluous error_free() Markus Armbruster
2020-06-22 10:42 ` [PATCH 13/22] aspeed: Clean up roundabout error propagation Markus Armbruster
2020-06-22 12:37 ` Cédric Le Goater
2020-06-22 10:42 ` [PATCH 14/22] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp Markus Armbruster
2020-06-22 10:42 ` [PATCH 15/22] qdev: Drop qbus_set_hotplug_handler() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 16/22] hw: Fix error API violation around object_property_set_link() Markus Armbruster
2020-06-22 12:47 ` Auger Eric
2020-06-22 21:22 ` Alistair Francis [this message]
2020-06-22 10:42 ` [PATCH 17/22] hw/arm: Drop useless object_property_set_link() error handling Markus Armbruster
2020-06-22 12:37 ` Cédric Le Goater
2020-06-22 10:42 ` [PATCH 18/22] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations Markus Armbruster
2020-06-22 10:42 ` Markus Armbruster
2020-06-22 21:23 ` Alistair Francis
2020-06-22 21:23 ` Alistair Francis
2020-06-22 10:42 ` [PATCH 19/22] riscv_hart: Fix riscv_harts_realize() " Markus Armbruster
2020-06-22 10:42 ` Markus Armbruster
2020-06-22 21:19 ` Alistair Francis
2020-06-22 21:19 ` Alistair Francis
2020-06-22 10:42 ` [PATCH 20/22] mips/cps: Fix mips_cps_realize() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 21/22] x86: Fix x86_cpu_new() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 22/22] amd_iommu: Fix amdvi_realize() error API violation Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKM2bUbgZD7gEFJ9A99dQ2zzCoSydOrp51pEeWmWCpE+bA@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=aleksandar.qemu.devel@gmail.com \
--cc=aleksandar.rikalo@syrmia.com \
--cc=alistair@alistair23.me \
--cc=armbru@redhat.com \
--cc=aurelien@aurel32.net \
--cc=edgar.iglesias@gmail.com \
--cc=eric.auger@redhat.com \
--cc=kraxel@redhat.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.