With code like
Object *obj = object_new(TYPE_BLAH)
the caller can be pretty confident that they will successfully create
an object instance of TYPE_BLAH. They know exactly what type has been
requested, so it passing an abstract type for example, it is a clear
programmer error that they'll get an assertion failure.
Conversely with code like
void somefunc(const char *typename) {
Object * obj = object_new(typename)
...
}
all bets are off, because the call of object_new() knows nothing
about what 'typename' resolves to. It could easily be an abstract
type. As a result, many code paths have added a manual check ahead
of time
if (object_class_is_abstract(typename)) {
error_setg(errp, ....)
}
...except for where we forget to do this, such as qdev_new().
Overall 'object_new' is a bad design because it is inherantly
unsafe to call with unvalidated typenames.
This problem is made worse by the proposal to introduce the idea
of 'singleton' classes[1].
Thus, this series suggests a way to improve safety at build
time. The core idea is to allow 'object_new' to continue to be
used *if-and-only-if* given a static, const string, because that
scenario indicates the caller is aware of what type they are
creating at build time.
A new 'object_new_dynamic' method is proposed for cases where
the typename is dynamically chosen at runtime. This method has
an "Error **errp" parameter, which can report when an abstract
type is created, leaving the assert()s only for scenarios which
are unambiguous programmer errors.
With a little macro magic, we guarantee a compile error is
generated if 'object_new' is called with a dynamic type, forcing
all potentially unsafe code over to object_new_dynamic.
This is more tractable than adding 'Error **errp' to 'object_new'
as only a handful of places use a dynamic type name.
NB, this is an RFC as it is not fully complete.
* I have only converted enough object_new -> object_new_dynamic
to make the x86_64-softmu target compile. It probably fails on
other targets.
* I have not run any test suites yet, so they may or may not pass
* I stubbed qdev_new to just pass &error_fatal. qdev_new needs
the same conceptual fix to introcce qdev_new_dynamic with
the macro magic to force its use
Obviously if there's agreement that this conceptual idea is valid,
then all these gaps would be fixed.
With this series, my objections to Peter Xu's singleton series[1]
would be largely nullified.
[1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html
Daniel P. Berrangé (5):
qom: refactor checking abstract property when creating instances
qom: allow failure of object_new_with_class
convert code to object_new_dynamic() where appropriate
qom: introduce object_new_dynamic()
qom: enforce use of static, const string with object_new()
accel/accel-user.c | 3 +-
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 +++-
include/qom/object.h | 48 ++++++++++++++++++++++++++++++--
net/net.c | 10 ++++---
qom/object.c | 38 +++++++++++++++++--------
qom/object_interfaces.c | 7 ++---
qom/qom-qmp-cmds.c | 15 ++++++----
system/vl.c | 6 ++--
target/i386/cpu-apic.c | 8 +++++-
target/i386/cpu-sysemu.c | 11 ++++++--
target/i386/cpu.c | 4 +--
target/s390x/cpu_models_sysemu.c | 7 +++--
tests/unit/check-qom-interface.c | 3 +-
tests/unit/test-smp-parse.c | 20 ++++++-------
21 files changed, 151 insertions(+), 61 deletions(-)
--
2.46.0
