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(-)
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 = {};
--
2.46.0
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. -- Peter Xu
On Thu, Oct 31, 2024 at 03:21:17PM -0400, Peter Xu wrote: > 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(). For the sake of other readers, here's what you already figured out from looking at patch 5... The end of this series will enforce that the argument to object_new() is a static, const string. It will become a compile error to pass a variable to object_new(), and thus all such cases need switching to object_new_dynamic(), even if they just end up passing in &error_abort in some cases. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.