[PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure

Markus Armbruster posted 46 patches 5 years, 5 months ago
There is a newer version of this series
[PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
Posted by Markus Armbruster 5 years, 5 months ago
Pass &error_abort instead of NULL where the returned value is
dereferenced or asserted to be non-null.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/platform-bus.c     | 5 +++--
 hw/ppc/spapr_drc.c         | 3 ++-
 hw/ppc/spapr_hcall.c       | 3 ++-
 hw/ppc/spapr_pci_nvlink2.c | 8 +++++---
 ui/vnc.c                   | 2 +-
 5 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index d494e5cec1..b700ff45fe 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -22,6 +22,7 @@
 #include "qemu/osdep.h"
 #include "hw/platform-bus.h"
 #include "hw/qdev-properties.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 
@@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
         return -1;
     }
 
-    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", NULL);
-
+    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
+                                         &error_abort);
     assert(parent_mr);
     if (parent_mr != pbus_mr_obj) {
         /* MMIO region is not mapped on platform bus */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index c8e8ba2ee8..43d12bc33a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -867,7 +867,8 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
             continue;
         }
 
-        obj = object_property_get_link(root_container, prop->name, NULL);
+        obj = object_property_get_link(root_container, prop->name,
+                                       &error_abort);
         drc = SPAPR_DR_CONNECTOR(obj);
         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 0f54988f2e..c1d01228c6 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1655,7 +1655,8 @@ static void spapr_handle_transient_dev_before_cas(SpaprMachineState *spapr)
             continue;
         }
         drc = SPAPR_DR_CONNECTOR(object_property_get_link(drc_container,
-                                                          prop->name, NULL));
+                                                          prop->name,
+                                                          &error_abort));
 
         if (spapr_drc_transient(drc)) {
             spapr_drc_reset(drc);
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 8332d5694e..e4e09a93e6 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, PCIDevice *pdev,
     if (tgt) {
         Error *local_err = NULL;
         SpaprPhbPciNvGpuConfig *nvgpus = opaque;
-        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
+        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
+                                                  &error_abort);
         Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
-                                                  NULL);
+                                                  &error_abort);
 
         g_assert(mr_gpu || mr_npu);
         if (mr_gpu) {
@@ -358,7 +359,8 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, void *fdt)
     for (i = 0; i < sphb->nvgpus->num; ++i) {
         SpaprPhbPciNvGpuSlot *nvslot = &sphb->nvgpus->slots[i];
         Object *nv_mrobj = object_property_get_link(OBJECT(nvslot->gpdev),
-                                                    "nvlink2-mr[0]", NULL);
+                                                    "nvlink2-mr[0]",
+                                                    &error_abort);
         uint32_t associativity[] = {
             cpu_to_be32(0x4),
             SPAPR_GPU_NUMA_ID,
diff --git a/ui/vnc.c b/ui/vnc.c
index 527ad25124..f006aa1afd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -568,7 +568,7 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
                        &info->vencrypt, &info->has_vencrypt);
         if (vd->dcl.con) {
             dev = DEVICE(object_property_get_link(OBJECT(vd->dcl.con),
-                                                  "device", NULL));
+                                                  "device", &error_abort));
             info->has_display = true;
             info->display = g_strdup(dev->id);
         }
-- 
2.26.2


Re: [PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
Posted by Eric Blake 5 years, 5 months ago
On 6/24/20 11:43 AM, Markus Armbruster wrote:
> Pass &error_abort instead of NULL where the returned value is
> dereferenced or asserted to be non-null.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> @@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
>           return -1;
>       }
>   
> -    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container", NULL);
> -
> +    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
> +                                         &error_abort);
>       assert(parent_mr);

Do we still need to keep the assert?

> +++ b/hw/ppc/spapr_pci_nvlink2.c
> @@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, PCIDevice *pdev,
>       if (tgt) {
>           Error *local_err = NULL;
>           SpaprPhbPciNvGpuConfig *nvgpus = opaque;
> -        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
> +        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
> +                                                  &error_abort);
>           Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
> -                                                  NULL);
> +                                                  &error_abort);
>   
>           g_assert(mr_gpu || mr_npu);

Likewise.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
Posted by Markus Armbruster 5 years, 5 months ago
Eric Blake <eblake@redhat.com> writes:

> On 6/24/20 11:43 AM, Markus Armbruster wrote:
>> Pass &error_abort instead of NULL where the returned value is
>> dereferenced or asserted to be non-null.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> @@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
>>           return -1;
>>       }
>>   -    parent_mr = object_property_get_link(OBJECT(sbdev_mr),
>> "container", NULL);
>> -
>> +    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
>> +                                         &error_abort);
>>       assert(parent_mr);
>
> Do we still need to keep the assert?

Not really, I guess.

>> +++ b/hw/ppc/spapr_pci_nvlink2.c
>> @@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, PCIDevice *pdev,
>>       if (tgt) {
>>           Error *local_err = NULL;
>>           SpaprPhbPciNvGpuConfig *nvgpus = opaque;
>> -        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
>> +        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
>> +                                                  &error_abort);
>>           Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
>> -                                                  NULL);
>> +                                                  &error_abort);
>>             g_assert(mr_gpu || mr_npu);
>
> Likewise.

I'll drop both unless somebody objects.


Re: [PATCH 33/46] qom: Crash more nicely on object_property_get_link() failure
Posted by Markus Armbruster 5 years, 5 months ago
Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 6/24/20 11:43 AM, Markus Armbruster wrote:
>>> Pass &error_abort instead of NULL where the returned value is
>>> dereferenced or asserted to be non-null.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>>> @@ -63,8 +64,8 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
>>>           return -1;
>>>       }
>>>   -    parent_mr = object_property_get_link(OBJECT(sbdev_mr),
>>> "container", NULL);
>>> -
>>> +    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
>>> +                                         &error_abort);
>>>       assert(parent_mr);
>>
>> Do we still need to keep the assert?
>
> Not really, I guess.
>
>>> +++ b/hw/ppc/spapr_pci_nvlink2.c
>>> @@ -141,9 +141,10 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, PCIDevice *pdev,
>>>       if (tgt) {
>>>           Error *local_err = NULL;
>>>           SpaprPhbPciNvGpuConfig *nvgpus = opaque;
>>> -        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
>>> +        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
>>> +                                                  &error_abort);
>>>           Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
>>> -                                                  NULL);
>>> +                                                  &error_abort);
>>>             g_assert(mr_gpu || mr_npu);
>>
>> Likewise.
>
> I'll drop both unless somebody objects.

The second hunk needs to be dropped instead: either of the two
object_property_get_link() may fail, just not both.