| Message ID | 20241031155350.3240361-1-berrange@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RFC: require error handling for dynamically created objects | expand |
On Thu, Oct 31, 2024 at 03:53:45PM +0000, Daniel P. Berrangé wrote: > 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 I suppose this is the only missing path in my patch 1 in qdev_new().. Even if we don't convert all of them, at least we can still convert the easiest (qdev_device_add_from_qdict()) so as to drop the abstract check in qdev_get_device_class(). The other one we can drop already after this series applied is the one in char_get_class(). I think after that, all explicit abstract checks for for object instantiations should be all gone.
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(-)