diff mbox series

[RFC,3/5] convert code to object_new_dynamic() where appropriate

Message ID 20241031155350.3240361-4-berrange@redhat.com (mailing list archive)
State New
Headers show
Series RFC: require error handling for dynamically created objects | expand

Commit Message

Daniel P. Berrangé Oct. 31, 2024, 3:53 p.m. UTC
In cases where object_new() is not being passed a static, const
string, the caller cannot be sure what type they are instantiating.
There is a risk that instantiation could fail, if it is an abstract
type.

Convert such cases over to use object_new_dynamic() such that they
are forced to expect failure. In some cases failure can be easily
propagated, but in others using &error_abort maintainers existing
behaviour.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char.c                   |  5 ++++-
 hw/core/bus.c                    |  2 +-
 hw/core/cpu-common.c             |  2 +-
 hw/core/qdev.c                   |  4 ++--
 hw/i386/x86-common.c             |  5 ++++-
 hw/i386/xen/xen-pvh.c            |  2 +-
 hw/vfio/common.c                 |  6 +++++-
 hw/vfio/container.c              |  6 +++++-
 qom/object_interfaces.c          |  7 ++-----
 qom/qom-qmp-cmds.c               | 15 +++++++++------
 tests/unit/check-qom-interface.c |  3 ++-
 tests/unit/test-smp-parse.c      | 20 ++++++++++----------
 12 files changed, 46 insertions(+), 31 deletions(-)

Comments

Peter Xu Oct. 31, 2024, 7:21 p.m. UTC | #1
On Thu, Oct 31, 2024 at 03:53:48PM +0000, Daniel P. Berrangé wrote:
> In cases where object_new() is not being passed a static, const
> string, the caller cannot be sure what type they are instantiating.
> There is a risk that instantiation could fail, if it is an abstract
> type.
> 
> Convert such cases over to use object_new_dynamic() such that they
> are forced to expect failure. In some cases failure can be easily
> propagated, but in others using &error_abort maintainers existing
> behaviour.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char.c                   |  5 ++++-
>  hw/core/bus.c                    |  2 +-
>  hw/core/cpu-common.c             |  2 +-
>  hw/core/qdev.c                   |  4 ++--
>  hw/i386/x86-common.c             |  5 ++++-
>  hw/i386/xen/xen-pvh.c            |  2 +-
>  hw/vfio/common.c                 |  6 +++++-
>  hw/vfio/container.c              |  6 +++++-
>  qom/object_interfaces.c          |  7 ++-----
>  qom/qom-qmp-cmds.c               | 15 +++++++++------
>  tests/unit/check-qom-interface.c |  3 ++-
>  tests/unit/test-smp-parse.c      | 20 ++++++++++----------
>  12 files changed, 46 insertions(+), 31 deletions(-)

I think we could leave the test cases alone without _dynamic(), because
they do test static types (even if they used "opaque"..), and they should
really (and forever) assert on failures..

IMHO we should keep _dynamic() usages small if we'd like to introduce it,
only in the paths where its failure will be properly handled.  Basically, I
think we shouldn't use _dynamic() if we know it'll be error_abort.. because
that's fundamentally identical to object_new().

A small set of _dynamic() usages also keep us easy to track all the paths
where QEMU can create some totally random objects too.
diff mbox series

Patch

diff --git a/chardev/char.c b/chardev/char.c
index a1722aa076..4de9a496a2 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -996,7 +996,10 @@  static Chardev *chardev_new(const char *id, const char *typename,
     assert(g_str_has_prefix(typename, "chardev-"));
     assert(id);
 
-    obj = object_new(typename);
+    if (!(obj = object_new_dynamic(typename, errp))) {
+        return NULL;
+    }
+    
     chr = CHARDEV(obj);
     chr->handover_yank_instance = handover_yank_instance;
     chr->label = g_strdup(id);
diff --git a/hw/core/bus.c b/hw/core/bus.c
index b9d89495cd..2de714b274 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -163,7 +163,7 @@  BusState *qbus_new(const char *typename, DeviceState *parent, const char *name)
 {
     BusState *bus;
 
-    bus = BUS(object_new(typename));
+    bus = BUS(object_new_dynamic(typename, &error_abort));
     qbus_init_internal(bus, parent, name);
 
     return bus;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 09c7903594..8768ae39ed 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -57,7 +57,7 @@  bool cpu_exists(int64_t id)
 CPUState *cpu_create(const char *typename)
 {
     Error *err = NULL;
-    CPUState *cpu = CPU(object_new(typename));
+    CPUState *cpu = CPU(object_new_dynamic(typename, &error_abort));
     if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
         error_report_err(err);
         object_unref(OBJECT(cpu));
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index db36f54d91..a143d8b721 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -162,7 +162,7 @@  DeviceState *qdev_new(const char *name)
         error_report("unknown type '%s'", name);
         abort();
     }
-    return DEVICE(object_new(name));
+    return DEVICE(object_new_dynamic(name, &error_abort));
 }
 
 DeviceState *qdev_try_new(const char *name)
@@ -170,7 +170,7 @@  DeviceState *qdev_try_new(const char *name)
     if (!module_object_class_by_name(name)) {
         return NULL;
     }
-    return DEVICE(object_new(name));
+    return DEVICE(object_new_dynamic(name, &error_abort));
 }
 
 static QTAILQ_HEAD(, DeviceListener) device_listeners
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index b86c38212e..b75443ec26 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -55,7 +55,10 @@  static size_t pvh_start_addr;
 
 static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
 {
-    Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
+    Object *cpu = object_new_dynamic(MACHINE(x86ms)->cpu_type, errp);
+    if (!cpu) {
+        return;
+    }
 
     if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
         goto out;
diff --git a/hw/i386/xen/xen-pvh.c b/hw/i386/xen/xen-pvh.c
index f1f02d3311..9aeb4e430b 100644
--- a/hw/i386/xen/xen-pvh.c
+++ b/hw/i386/xen/xen-pvh.c
@@ -28,7 +28,7 @@  struct XenPVHx86State {
 static DeviceState *xen_pvh_cpu_new(MachineState *ms,
                                     int64_t apic_id)
 {
-    Object *cpu = object_new(ms->cpu_type);
+    Object *cpu = object_new_dynamic(ms->cpu_type, &error_abort);
 
     object_property_add_child(OBJECT(ms), "cpu[*]", cpu);
     object_property_set_uint(cpu, "apic-id", apic_id, &error_fatal);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 36d0cf6585..d57097a194 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1550,7 +1550,11 @@  bool vfio_attach_device(char *name, VFIODevice *vbasedev,
 
 
     if (!vbasedev->mdev) {
-        hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
+        Object *obj = object_new_dynamic(ops->hiod_typename, errp);
+        if (!obj) {
+            return false;
+        }
+        hiod = HOST_IOMMU_DEVICE(obj);
         vbasedev->hiod = hiod;
     }
 
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 9ccdb639ac..5964009a3c 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -419,6 +419,7 @@  static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
 {
     int iommu_type;
     const char *vioc_name;
+    Object *obj;
     VFIOContainer *container;
 
     iommu_type = vfio_get_iommu_type(fd, errp);
@@ -432,7 +433,10 @@  static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
 
     vioc_name = vfio_get_iommu_class_name(iommu_type);
 
-    container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
+    if (!(obj = object_new_dynamic(vioc_name, NULL))) {
+        return NULL;
+    }
+    container = VFIO_IOMMU_LEGACY(obj);
     container->fd = fd;
     container->iommu_type = iommu_type;
     return container;
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e0833c8bfe..756fca3649 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -102,13 +102,10 @@  Object *user_creatable_add_type(const char *type, const char *id,
         return NULL;
     }
 
-    if (object_class_is_abstract(klass)) {
-        error_setg(errp, "object type '%s' is abstract", type);
+    assert(qdict);
+    if (!(obj = object_new_dynamic(type, errp))) {
         return NULL;
     }
-
-    assert(qdict);
-    obj = object_new(type);
     object_set_properties_from_qdict(obj, qdict, v, &local_err);
     if (local_err) {
         goto out;
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index e91a235347..b1590231d7 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -134,14 +134,15 @@  ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
         return NULL;
     }
 
-    if (!object_class_dynamic_cast(klass, TYPE_DEVICE)
-        || object_class_is_abstract(klass)) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
-                   "a non-abstract device type");
+    if (!object_class_dynamic_cast(klass, TYPE_DEVICE)) {
+        error_setg(errp, "Object type '%s' is not a device",
+                   typename);
         return NULL;
     }
 
-    obj = object_new(typename);
+    if (!(obj = object_new_dynamic(typename, errp))) {
+        return NULL;
+    }
 
     object_property_iter_init(&iter, obj);
     while ((prop = object_property_iter_next(&iter))) {
@@ -202,7 +203,9 @@  ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
     if (object_class_is_abstract(klass)) {
         object_class_property_iter_init(&iter, klass);
     } else {
-        obj = object_new(typename);
+        if (!(obj = object_new_dynamic(typename, errp))) {
+            return NULL;
+        }
         object_property_iter_init(&iter, obj);
     }
     while ((prop = object_property_iter_next(&iter))) {
diff --git a/tests/unit/check-qom-interface.c b/tests/unit/check-qom-interface.c
index c99be97ed8..e4532ae814 100644
--- a/tests/unit/check-qom-interface.c
+++ b/tests/unit/check-qom-interface.c
@@ -13,6 +13,7 @@ 
 
 #include "qom/object.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 
 
 #define TYPE_TEST_IF "test-interface"
@@ -67,7 +68,7 @@  static const TypeInfo intermediate_impl_info = {
 
 static void test_interface_impl(const char *type)
 {
-    Object *obj = object_new(type);
+    Object *obj = object_new_dynamic(type, &error_abort);
     TestIf *iobj = TEST_IF(obj);
     TestIfClass *ioc = TEST_IF_GET_CLASS(iobj);
 
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index f9bccb56ab..f4893d5f24 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -1008,7 +1008,7 @@  static void machine_full_topo_class_init(ObjectClass *oc, void *data)
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1027,7 +1027,7 @@  static void test_generic_valid(const void *opaque)
 static void test_generic_invalid(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1046,7 +1046,7 @@  static void test_generic_invalid(const void *opaque)
 static void test_with_modules(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1096,7 +1096,7 @@  static void test_with_modules(const void *opaque)
 static void test_with_dies(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1146,7 +1146,7 @@  static void test_with_dies(const void *opaque)
 static void test_with_modules_dies(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1207,7 +1207,7 @@  static void test_with_modules_dies(const void *opaque)
 static void test_with_clusters(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1257,7 +1257,7 @@  static void test_with_clusters(const void *opaque)
 static void test_with_books(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1307,7 +1307,7 @@  static void test_with_books(const void *opaque)
 static void test_with_drawers(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1357,7 +1357,7 @@  static void test_with_drawers(const void *opaque)
 static void test_with_drawers_books(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1418,7 +1418,7 @@  static void test_with_drawers_books(const void *opaque)
 static void test_full_topo(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};