[PATCH 16/22] hw: Fix error API violation around object_property_set_link()

Markus Armbruster posted 22 patches 5 years, 7 months ago
Maintainers: Jason Wang <jasowang@redhat.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Max Reitz <mreitz@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Richard Henderson <rth@twiddle.net>, "Michael S. Tsirkin" <mst@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Markus Armbruster <armbru@redhat.com>, Eric Auger <eric.auger@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, Christian Schoenebeck <qemu_oss@crudebyte.com>, Aurelien Jarno <aurelien@aurel32.net>, Alistair Francis <alistair@alistair23.me>, Paolo Bonzini <pbonzini@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Eduardo Habkost <ehabkost@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Michael Roth <mdroth@linux.vnet.ibm.com>, Peter Maydell <peter.maydell@linaro.org>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Kevin Wolf <kwolf@redhat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Xie Changlong <xiechanglong.d@gmail.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Wen Congyang <wencongyang2@huawei.com>
There is a newer version of this series
[PATCH 16/22] hw: Fix error API violation around object_property_set_link()
Posted by Markus Armbruster 5 years, 7 months ago
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>
---
 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


Re: [PATCH 16/22] hw: Fix error API violation around object_property_set_link()
Posted by Auger Eric 5 years, 7 months ago
Hi Markus,
On 6/22/20 12:42 PM, Markus Armbruster 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.
s/.check/.set()? in object_property_set()
> 
> 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>

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


> ---
>  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);
>  }
>  
> 


Re: [PATCH 16/22] hw: Fix error API violation around object_property_set_link()
Posted by Alistair Francis 5 years, 7 months ago
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
>
>