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

2020-07-04 Thread Markus Armbruster 
Vladimir Sementsov-Ogievskiy  writes:

> 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 
>> ---
>>   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.


Mission creep :)

We have many, many object_property_get_FOO() and _set_FOO() calls that
ignore errors.  Probably most of them should abort on error instead.
Let's leave that for a future series.

> with it or not:
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Thanks!

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

2020-07-03 Thread 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 
---
  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 


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

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

2020-07-02 Thread Eric Blake 

On 7/2/20 10:49 AM, 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 
---
  hw/core/platform-bus.c | 6 +++---

Reviewed-by: Eric Blake 

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

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

2020-07-02 Thread Markus Armbruster 
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 
---
 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,
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