Re: [PATCH v2 23/44] qom: Crash more nicely on object_property_get_link() failure

[Vladimir Sementsov-Ogievskiy]

02.07.2020 18:49, Markus Armbruster wrote:
> Pass &error_abort instead of NULL where the returned value is
> dereferenced or asserted to be non-null.  Drop a now redundant
> assertion.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/core/platform-bus.c     | 6 +++---
>   hw/ppc/spapr_drc.c         | 3 ++-
>   hw/ppc/spapr_hcall.c       | 3 ++-
>   hw/ppc/spapr_pci_nvlink2.c | 3 ++-
>   ui/vnc.c                   | 2 +-
>   5 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
> index d494e5cec1..5037ca265e 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,9 +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);
> -
> -    assert(parent_mr);
> +    parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
> +                                         &error_abort);
>       if (parent_mr != pbus_mr_obj) {
>           /* MMIO region is not mapped on platform bus */
>           return -1;
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index d10193f39e..1f18b79348 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -870,7 +870,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..dd8cd6db96 100644
> --- a/hw/ppc/spapr_pci_nvlink2.c
> +++ b/hw/ppc/spapr_pci_nvlink2.c
> @@ -358,7 +358,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,

the following
uint64_t size = object_property_get_uint(nv_mrobj, "size", NULL);

is a good candidate for error_abort as well.

with it or not:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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


--
Best regards,
Vladimir