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