| Message ID | 20241024165627.1372621-2-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | QOM: Singleton interface | expand |
Hi Peter, (Cc'ing Mark) On 24/10/24 13:56, Peter Xu wrote: > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > qom/object.c | 3 +++ > qom/object_interfaces.c | 24 +++++++++++++++++ > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > system/qdev-monitor.c | 7 +++++ > 5 files changed, 100 insertions(+), 3 deletions(-) > +/** > + * SingletonClass: > + * > + * @parent_class: the base class > + * @get_instance: fetch the singleton instance if it is created, > + * NULL otherwise. > + * > + * Singleton class describes the type of object classes that can only > + * provide one instance for the whole lifecycle of QEMU. It will fail the > + * operation if one attemps to create more than one instance. > + * > + * One can fetch the single object using class's get_instance() callback if > + * it was created before. This can be useful for operations like QMP > + * qom-list-properties, where dynamically creating an object might not be > + * feasible. > + */ > +struct SingletonClass { > + /* <private> */ > + InterfaceClass parent_class; > + /* <public> */ > + Object *(*get_instance)(Error **errp); IMHO asking get_instance() is overkill ... > +}; > + > +/** > + * object_class_is_singleton: > + * > + * @class: the class to detect singleton > + * > + * Returns: true if it's a singleton class, false otherwise. > + */ > +bool object_class_is_singleton(ObjectClass *class); > + > +/** > + * singleton_get_instance: > + * > + * @class: the class to fetch singleton instance > + * > + * Returns: the object* if the class is a singleton class and the singleton > + * object is created, NULL otherwise. > + */ > +Object *singleton_get_instance(ObjectClass *class); > + > #endif > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > index e0833c8bfe..6766060d0a 100644 > --- a/qom/object_interfaces.c > +++ b/qom/object_interfaces.c > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) > object_unparent(object_get_objects_root()); > } > > +bool object_class_is_singleton(ObjectClass *class) > +{ > + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); > +} > + > +Object *singleton_get_instance(ObjectClass *class) > +{ ... when we can use object_resolve_type_unambiguous: return object_resolve_type_unambiguous(object_class_get_name(class, NULL)); BTW should we pass Error** argument to singleton_get_instance()? > + SingletonClass *singleton = > + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); > + > + if (!singleton) { > + return NULL; > + } > + > + return singleton->get_instance(&error_abort); > +} Alternatively call object_resolve_type_unambiguous() in instance_init()? Regards, Phil.
On Thu, Oct 24, 2024 at 05:02:19PM -0300, Philippe Mathieu-Daudé wrote: > Hi Peter, Hi, Phil, Thanks for the quick reviews! > > (Cc'ing Mark) > > On 24/10/24 13:56, Peter Xu wrote: > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > > qom/object.c | 3 +++ > > qom/object_interfaces.c | 24 +++++++++++++++++ > > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > > system/qdev-monitor.c | 7 +++++ > > 5 files changed, 100 insertions(+), 3 deletions(-) > > > > +/** > > + * SingletonClass: > > + * > > + * @parent_class: the base class > > + * @get_instance: fetch the singleton instance if it is created, > > + * NULL otherwise. > > + * > > + * Singleton class describes the type of object classes that can only > > + * provide one instance for the whole lifecycle of QEMU. It will fail the > > + * operation if one attemps to create more than one instance. > > + * > > + * One can fetch the single object using class's get_instance() callback if > > + * it was created before. This can be useful for operations like QMP > > + * qom-list-properties, where dynamically creating an object might not be > > + * feasible. > > + */ > > +struct SingletonClass { > > + /* <private> */ > > + InterfaceClass parent_class; > > + /* <public> */ > > + Object *(*get_instance)(Error **errp); > > IMHO asking get_instance() is overkill ... > > > +}; > > + > > +/** > > + * object_class_is_singleton: > > + * > > + * @class: the class to detect singleton > > + * > > + * Returns: true if it's a singleton class, false otherwise. > > + */ > > +bool object_class_is_singleton(ObjectClass *class); > > + > > +/** > > + * singleton_get_instance: > > + * > > + * @class: the class to fetch singleton instance > > + * > > + * Returns: the object* if the class is a singleton class and the singleton > > + * object is created, NULL otherwise. > > + */ > > +Object *singleton_get_instance(ObjectClass *class); > > + > > #endif > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > > index e0833c8bfe..6766060d0a 100644 > > --- a/qom/object_interfaces.c > > +++ b/qom/object_interfaces.c > > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) > > object_unparent(object_get_objects_root()); > > } > > +bool object_class_is_singleton(ObjectClass *class) > > +{ > > + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); > > +} > > + > > +Object *singleton_get_instance(ObjectClass *class) > > +{ > > ... when we can use object_resolve_type_unambiguous: > > return object_resolve_type_unambiguous(object_class_get_name(class, > NULL)); I think an issue is migration object is nowhere to be find under object_get_root(), so it won't work there. A side benefit is, it's also faster.. How about I use object_resolve_type_unambiguous() as a fallback? Then it's used only if get_instance() is not provided. > > BTW should we pass Error** argument to singleton_get_instance()? I didn't expect it to fail, hence I didn't even add it to make it more like "this will always success or it asserts" kind of things. I left Error** in the hook just to be slightly flexible, but I always pass in error_abort() in this patch. I can either add Error** if anyone thinks it useful, or even drop Error** in ->get_instance() since it's mostly not used at least for now. Let me know! > > > + SingletonClass *singleton = > > + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); > > + > > + if (!singleton) { > > + return NULL; > > + } > > + > > + return singleton->get_instance(&error_abort); > > +} > > Alternatively call object_resolve_type_unambiguous() in instance_init()? Thanks,
Peter Xu <peterx@redhat.com> writes: > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > qom/object.c | 3 +++ > qom/object_interfaces.c | 24 +++++++++++++++++ > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > system/qdev-monitor.c | 7 +++++ > 5 files changed, 100 insertions(+), 3 deletions(-) > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h > index 02b11a7ef0..9b2cc0e554 100644 > --- a/include/qom/object_interfaces.h > +++ b/include/qom/object_interfaces.h > @@ -177,4 +177,51 @@ bool user_creatable_del(const char *id, Error **errp); > */ > void user_creatable_cleanup(void); > > +#define TYPE_SINGLETON "singleton" > + > +typedef struct SingletonClass SingletonClass; > +DECLARE_CLASS_CHECKERS(SingletonClass, SINGLETON, TYPE_SINGLETON) > + > +/** > + * SingletonClass: > + * > + * @parent_class: the base class > + * @get_instance: fetch the singleton instance if it is created, > + * NULL otherwise. > + * > + * Singleton class describes the type of object classes that can only > + * provide one instance for the whole lifecycle of QEMU. It will fail the > + * operation if one attemps to create more than one instance. > + * > + * One can fetch the single object using class's get_instance() callback if > + * it was created before. This can be useful for operations like QMP > + * qom-list-properties, where dynamically creating an object might not be > + * feasible. > + */ > +struct SingletonClass { > + /* <private> */ > + InterfaceClass parent_class; > + /* <public> */ > + Object *(*get_instance)(Error **errp); > +}; > + > +/** > + * object_class_is_singleton: > + * > + * @class: the class to detect singleton > + * > + * Returns: true if it's a singleton class, false otherwise. > + */ > +bool object_class_is_singleton(ObjectClass *class); > + > +/** > + * singleton_get_instance: > + * > + * @class: the class to fetch singleton instance > + * > + * Returns: the object* if the class is a singleton class and the singleton > + * object is created, NULL otherwise. > + */ > +Object *singleton_get_instance(ObjectClass *class); A non-null return value can become dangling when the object gets destroyed. This could conceivably happen in another thread. The obviously safe interface would take a reference the caller must unref when done. > + > #endif > diff --git a/qom/object.c b/qom/object.c > index 11424cf471..ded299ae1a 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type > g_assert(type->abstract == false); > g_assert(size >= type->instance_size); > > + /* Singleton class can only create one object */ > + g_assert(!singleton_get_instance(type->class)); > + > memset(obj, 0, type->instance_size); > obj->class = type->class; > object_ref(obj); > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > index e0833c8bfe..6766060d0a 100644 > --- a/qom/object_interfaces.c > +++ b/qom/object_interfaces.c > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) > object_unparent(object_get_objects_root()); > } > > +bool object_class_is_singleton(ObjectClass *class) > +{ > + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); > +} > + > +Object *singleton_get_instance(ObjectClass *class) > +{ > + SingletonClass *singleton = > + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); > + > + if (!singleton) { > + return NULL; > + } > + > + return singleton->get_instance(&error_abort); > +} > + > static void register_types(void) > { > static const TypeInfo uc_interface_info = { > @@ -362,7 +379,14 @@ static void register_types(void) > .class_size = sizeof(UserCreatableClass), > }; > > + static const TypeInfo singleton_interface_info = { > + .name = TYPE_SINGLETON, > + .parent = TYPE_INTERFACE, > + .class_size = sizeof(SingletonClass), > + }; > + > type_register_static(&uc_interface_info); > + type_register_static(&singleton_interface_info); > } > > type_init(register_types) > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > index e91a235347..ecc1cf781c 100644 > --- a/qom/qom-qmp-cmds.c > +++ b/qom/qom-qmp-cmds.c > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > ObjectProperty *prop; > ObjectPropertyIterator iter; > ObjectPropertyInfoList *prop_list = NULL; > + bool create; > > klass = module_object_class_by_name(typename); > if (klass == NULL) { > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > return NULL; > } > > - obj = object_new(typename); > + /* Avoid creating multiple instances if the class is a singleton */ > + create = !object_class_is_singleton(klass) || > + !singleton_get_instance(klass); > + > + if (create) { > + obj = object_new(typename); If the class is not a singleton or else if no instance exists, we create a temporary instance. > + } else { > + obj = singleton_get_instance(klass); If the class is a singleton and the instance exists, we use that instead. Any properties the instance has created dynamically after object_new() are visible to introspection. This is not the case when we create a temporary instance. Such subtle differences are problematic. If we decide we're okay with this one, we need to document it. > + } > > object_property_iter_init(&iter, obj); > while ((prop = object_property_iter_next(&iter))) { > @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > QAPI_LIST_PREPEND(prop_list, info); > } > > - object_unref(obj); > + if (create) { > + object_unref(obj); > + } > > return prop_list; > } > @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, > return NULL; > } > > - if (object_class_is_abstract(klass)) { > + /* > + * Abstract classes are not for instantiations, meanwhile avoid > + * creating temporary singleton objects because it can cause conflicts > + * if there's already one created. > + */ > + if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) { > object_class_property_iter_init(&iter, klass); > } else { > obj = object_new(typename); If the class is a singleton, we treat it as if it was abstract. Its instance properties, if any, are not visible in qom-list-properties. This is a defect. If you make an existing class with instance properties a singleton, the defect is a regression. > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c > index 44994ea0e1..1310f35c9f 100644 > --- a/system/qdev-monitor.c > +++ b/system/qdev-monitor.c > @@ -36,6 +36,7 @@ > #include "qemu/option.h" > #include "qemu/qemu-print.h" > #include "qemu/option_int.h" > +#include "qom/object_interfaces.h" > #include "sysemu/block-backend.h" > #include "migration/misc.h" > #include "qemu/cutils.h" > @@ -643,6 +644,12 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, > return NULL; > } > > + if (singleton_get_instance(OBJECT_CLASS(dc))) { > + error_setg(errp, "Class '%s' only supports one instance", > + driver); > + return NULL; > + } > + > /* find bus */ > path = qdict_get_try_str(opts, "bus"); > if (path != NULL) {
On Thu, Oct 24, 2024 at 12:56:24PM -0400, Peter Xu wrote: Adding significant new functionality to QOM should really come with a commit message explaining the rationale and the design choices > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > qom/object.c | 3 +++ > qom/object_interfaces.c | 24 +++++++++++++++++ > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > system/qdev-monitor.c | 7 +++++ > 5 files changed, 100 insertions(+), 3 deletions(-) snip > + * Singleton class describes the type of object classes that can only > + * provide one instance for the whole lifecycle of QEMU. It will fail the > + * operation if one attemps to create more than one instance. > + * > + * One can fetch the single object using class's get_instance() callback if > + * it was created before. This can be useful for operations like QMP > + * qom-list-properties, where dynamically creating an object might not be > + * feasible. snip > +/** > + * singleton_get_instance: > + * > + * @class: the class to fetch singleton instance > + * > + * Returns: the object* if the class is a singleton class and the singleton > + * object is created, NULL otherwise. > + */ > +Object *singleton_get_instance(ObjectClass *class); With this design, all code that uses a given type needs to know whether or not it is intended to be a singleton. If some code somewhere mistakenly calls 'object_new' instead of 'singleton_get_instance', the singleton type is no longer a singleton, except you handle this by adding an assert in object_initialize_with_type. This is still a bit of a loaded foot-gun IMHO, as we don't want random code asserting. > diff --git a/qom/object.c b/qom/object.c > index 11424cf471..ded299ae1a 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type > g_assert(type->abstract == false); > g_assert(size >= type->instance_size); > > + /* Singleton class can only create one object */ > + g_assert(!singleton_get_instance(type->class)); > + > memset(obj, 0, type->instance_size); > obj->class = type->class; > object_ref(obj); > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > index e91a235347..ecc1cf781c 100644 > --- a/qom/qom-qmp-cmds.c > +++ b/qom/qom-qmp-cmds.c > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > ObjectProperty *prop; > ObjectPropertyIterator iter; > ObjectPropertyInfoList *prop_list = NULL; > + bool create; > > klass = module_object_class_by_name(typename); > if (klass == NULL) { > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > return NULL; > } > > - obj = object_new(typename); > + /* Avoid creating multiple instances if the class is a singleton */ > + create = !object_class_is_singleton(klass) || > + !singleton_get_instance(klass); > + > + if (create) { > + obj = object_new(typename); > + } else { > + obj = singleton_get_instance(klass); > + } This is the first foot-gun example. I really strongly dislike that the design pattern forces us to create code like this, as we can never be confident we've correctly identified all the places which may call object_new on a singleton... > @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > QAPI_LIST_PREPEND(prop_list, info); > } > > - object_unref(obj); > + if (create) { > + object_unref(obj); > + } ...and this just compounds the ugliness. > @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, > return NULL; > } > > - if (object_class_is_abstract(klass)) { > + /* > + * Abstract classes are not for instantiations, meanwhile avoid > + * creating temporary singleton objects because it can cause conflicts > + * if there's already one created. > + */ Another example of the foot-gun firing at random code > + if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) { > object_class_property_iter_init(&iter, klass); > } else { > obj = object_new(typename); With changes to QOM, I think it is generally informative to look at how GLib has handled the problem, since the QOM design has heavily borrowed from its GObject design. In GObject, singletons are handled in a very differnt way. It has a concept of a "constructor" function against the class, which is what is responsible for allocating the object. By default the 'constructor' will call g_new0, but a class which wishes to become a singleton will override the 'constructor' function to allocate on first call, and return the cached object on subsequent calls. This is illustrated here: https://gitlab.gnome.org/GNOME/glib/-/blob/main/gobject/gobject.h#L297 The key benefit of this is that everything can carry on calling g_object_new() as before, as it will just "do the right thing" in terms of allocation. In QOM, we don't have a 'constructor' class function, we just directly call g_malloc from object_new_with_type. This is because at the time, we didn't see an immediate need for it. We could easily change that though to introduce the concept of a 'constructor', which could probably make singletons work without needing updates to existing code. With regards, Daniel
On 24/10/24 17:53, Peter Xu wrote: > On Thu, Oct 24, 2024 at 05:02:19PM -0300, Philippe Mathieu-Daudé wrote: >> Hi Peter, > > Hi, Phil, > > Thanks for the quick reviews! > >> >> (Cc'ing Mark) >> >> On 24/10/24 13:56, Peter Xu wrote: >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ >>> qom/object.c | 3 +++ >>> qom/object_interfaces.c | 24 +++++++++++++++++ >>> qom/qom-qmp-cmds.c | 22 ++++++++++++--- >>> system/qdev-monitor.c | 7 +++++ >>> 5 files changed, 100 insertions(+), 3 deletions(-) >> >> >>> +/** >>> + * SingletonClass: >>> + * >>> + * @parent_class: the base class >>> + * @get_instance: fetch the singleton instance if it is created, >>> + * NULL otherwise. >>> + * >>> + * Singleton class describes the type of object classes that can only >>> + * provide one instance for the whole lifecycle of QEMU. It will fail the >>> + * operation if one attemps to create more than one instance. >>> + * >>> + * One can fetch the single object using class's get_instance() callback if >>> + * it was created before. This can be useful for operations like QMP >>> + * qom-list-properties, where dynamically creating an object might not be >>> + * feasible. >>> + */ >>> +struct SingletonClass { >>> + /* <private> */ >>> + InterfaceClass parent_class; >>> + /* <public> */ >>> + Object *(*get_instance)(Error **errp); >> >> IMHO asking get_instance() is overkill ... >> >>> +}; >>> + >>> +/** >>> + * object_class_is_singleton: >>> + * >>> + * @class: the class to detect singleton >>> + * >>> + * Returns: true if it's a singleton class, false otherwise. >>> + */ >>> +bool object_class_is_singleton(ObjectClass *class); >>> + >>> +/** >>> + * singleton_get_instance: >>> + * >>> + * @class: the class to fetch singleton instance >>> + * >>> + * Returns: the object* if the class is a singleton class and the singleton >>> + * object is created, NULL otherwise. >>> + */ >>> +Object *singleton_get_instance(ObjectClass *class); >>> + >>> #endif >> >>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c >>> index e0833c8bfe..6766060d0a 100644 >>> --- a/qom/object_interfaces.c >>> +++ b/qom/object_interfaces.c >>> @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) >>> object_unparent(object_get_objects_root()); >>> } >>> +bool object_class_is_singleton(ObjectClass *class) >>> +{ >>> + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); >>> +} >>> + >>> +Object *singleton_get_instance(ObjectClass *class) >>> +{ >> >> ... when we can use object_resolve_type_unambiguous: >> >> return object_resolve_type_unambiguous(object_class_get_name(class, >> NULL)); > > I think an issue is migration object is nowhere to be find under > object_get_root(), so it won't work there. A side benefit is, it's also > faster.. Maybe a simpler alternative is to add a field in ObjectClass, maintain a single GHashTable to store TypeName -> Instance and retrieve as: Object *singleton_get_instance(ObjectClass *class) { return g_hash_table_lookup(&singletons, object_class_get_name(class)); } TBH the TYPE_SINGLETON interface seems a bit over-engineered and its implementation error prone. > How about I use object_resolve_type_unambiguous() as a fallback? Then it's > used only if get_instance() is not provided. > >> >> BTW should we pass Error** argument to singleton_get_instance()? > > I didn't expect it to fail, hence I didn't even add it to make it more like > "this will always success or it asserts" kind of things. I left Error** in > the hook just to be slightly flexible, but I always pass in error_abort() > in this patch. > > I can either add Error** if anyone thinks it useful, or even drop Error** > in ->get_instance() since it's mostly not used at least for now. > > Let me know! > >> >>> + SingletonClass *singleton = >>> + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); >>> + >>> + if (!singleton) { >>> + return NULL; >>> + } >>> + >>> + return singleton->get_instance(&error_abort); >>> +} >> >> Alternatively call object_resolve_type_unambiguous() in instance_init()? > > Thanks, >
On Fri, Oct 25, 2024 at 10:07:59AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > > qom/object.c | 3 +++ > > qom/object_interfaces.c | 24 +++++++++++++++++ > > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > > system/qdev-monitor.c | 7 +++++ > > 5 files changed, 100 insertions(+), 3 deletions(-) > > > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h > > index 02b11a7ef0..9b2cc0e554 100644 > > --- a/include/qom/object_interfaces.h > > +++ b/include/qom/object_interfaces.h > > @@ -177,4 +177,51 @@ bool user_creatable_del(const char *id, Error **errp); > > */ > > void user_creatable_cleanup(void); > > > > +#define TYPE_SINGLETON "singleton" > > + > > +typedef struct SingletonClass SingletonClass; > > +DECLARE_CLASS_CHECKERS(SingletonClass, SINGLETON, TYPE_SINGLETON) > > + > > +/** > > + * SingletonClass: > > + * > > + * @parent_class: the base class > > + * @get_instance: fetch the singleton instance if it is created, > > + * NULL otherwise. > > + * > > + * Singleton class describes the type of object classes that can only > > + * provide one instance for the whole lifecycle of QEMU. It will fail the > > + * operation if one attemps to create more than one instance. > > + * > > + * One can fetch the single object using class's get_instance() callback if > > + * it was created before. This can be useful for operations like QMP > > + * qom-list-properties, where dynamically creating an object might not be > > + * feasible. > > + */ > > +struct SingletonClass { > > + /* <private> */ > > + InterfaceClass parent_class; > > + /* <public> */ > > + Object *(*get_instance)(Error **errp); > > +}; > > + > > +/** > > + * object_class_is_singleton: > > + * > > + * @class: the class to detect singleton > > + * > > + * Returns: true if it's a singleton class, false otherwise. > > + */ > > +bool object_class_is_singleton(ObjectClass *class); > > + > > +/** > > + * singleton_get_instance: > > + * > > + * @class: the class to fetch singleton instance > > + * > > + * Returns: the object* if the class is a singleton class and the singleton > > + * object is created, NULL otherwise. > > + */ > > +Object *singleton_get_instance(ObjectClass *class); > > A non-null return value can become dangling when the object gets > destroyed. This could conceivably happen in another thread. > > The obviously safe interface would take a reference the caller must > unref when done. Ouch, thanks for spotting this! Yes we definitely need a refcount at least.. > > > + > > #endif > > diff --git a/qom/object.c b/qom/object.c > > index 11424cf471..ded299ae1a 100644 > > --- a/qom/object.c > > +++ b/qom/object.c > > @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type > > g_assert(type->abstract == false); > > g_assert(size >= type->instance_size); > > > > + /* Singleton class can only create one object */ > > + g_assert(!singleton_get_instance(type->class)); > > + > > memset(obj, 0, type->instance_size); > > obj->class = type->class; > > object_ref(obj); > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > > index e0833c8bfe..6766060d0a 100644 > > --- a/qom/object_interfaces.c > > +++ b/qom/object_interfaces.c > > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) > > object_unparent(object_get_objects_root()); > > } > > > > +bool object_class_is_singleton(ObjectClass *class) > > +{ > > + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); > > +} > > + > > +Object *singleton_get_instance(ObjectClass *class) > > +{ > > + SingletonClass *singleton = > > + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); > > + > > + if (!singleton) { > > + return NULL; > > + } > > + > > + return singleton->get_instance(&error_abort); > > +} > > + > > static void register_types(void) > > { > > static const TypeInfo uc_interface_info = { > > @@ -362,7 +379,14 @@ static void register_types(void) > > .class_size = sizeof(UserCreatableClass), > > }; > > > > + static const TypeInfo singleton_interface_info = { > > + .name = TYPE_SINGLETON, > > + .parent = TYPE_INTERFACE, > > + .class_size = sizeof(SingletonClass), > > + }; > > + > > type_register_static(&uc_interface_info); > > + type_register_static(&singleton_interface_info); > > } > > > > type_init(register_types) > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > > index e91a235347..ecc1cf781c 100644 > > --- a/qom/qom-qmp-cmds.c > > +++ b/qom/qom-qmp-cmds.c > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > ObjectProperty *prop; > > ObjectPropertyIterator iter; > > ObjectPropertyInfoList *prop_list = NULL; > > + bool create; > > > > klass = module_object_class_by_name(typename); > > if (klass == NULL) { > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > return NULL; > > } > > > > - obj = object_new(typename); > > + /* Avoid creating multiple instances if the class is a singleton */ > > + create = !object_class_is_singleton(klass) || > > + !singleton_get_instance(klass); > > + > > + if (create) { > > + obj = object_new(typename); > > If the class is not a singleton or else if no instance exists, we create > a temporary instance. > > > + } else { > > + obj = singleton_get_instance(klass); > > If the class is a singleton and the instance exists, we use that > instead. > > Any properties the instance has created dynamically after object_new() > are visible to introspection. This is not the case when we create a > temporary instance. Such subtle differences are problematic. If we > decide we're okay with this one, we need to document it. I am thinking what's the major use case for device-list-properties. If it's for mgmt to query a device property list for set/get before operating on a real instance (which represents already somewhere in the device tree), I hope it's fine. Basically, it means whatever queried here can always be used with qom-get/qom-set for this singleton as long as it's already created and present. I hope Libvirt cannot cache this results anyway, because we already have: ## # @device-list-properties: # ... # .. note:: Objects can create properties at runtime, for example to # describe links between different devices and/or objects. These # properties are not included in the output of this command. So I think it means mgmt cannot cache these results, but only query before qom-set/qom-get to make sure it's valid. In that case, maybe we can change that to s/are not included/may not be included/? > > > + } > > > > object_property_iter_init(&iter, obj); > > while ((prop = object_property_iter_next(&iter))) { > > @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > QAPI_LIST_PREPEND(prop_list, info); > > } > > > > - object_unref(obj); > > + if (create) { > > + object_unref(obj); > > + } > > > > return prop_list; > > } > > @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, > > return NULL; > > } > > > > - if (object_class_is_abstract(klass)) { > > + /* > > + * Abstract classes are not for instantiations, meanwhile avoid > > + * creating temporary singleton objects because it can cause conflicts > > + * if there's already one created. > > + */ > > + if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) { > > object_class_property_iter_init(&iter, klass); > > } else { > > obj = object_new(typename); > > If the class is a singleton, we treat it as if it was abstract. Its > instance properties, if any, are not visible in qom-list-properties. > This is a defect. If you make an existing class with instance > properties a singleton, the defect is a regression. I can switch to get_instance() for singleton when it's available. Would that work? Thanks! > > > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c > > index 44994ea0e1..1310f35c9f 100644 > > --- a/system/qdev-monitor.c > > +++ b/system/qdev-monitor.c > > @@ -36,6 +36,7 @@ > > #include "qemu/option.h" > > #include "qemu/qemu-print.h" > > #include "qemu/option_int.h" > > +#include "qom/object_interfaces.h" > > #include "sysemu/block-backend.h" > > #include "migration/misc.h" > > #include "qemu/cutils.h" > > @@ -643,6 +644,12 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, > > return NULL; > > } > > > > + if (singleton_get_instance(OBJECT_CLASS(dc))) { > > + error_setg(errp, "Class '%s' only supports one instance", > > + driver); > > + return NULL; > > + } > > + > > /* find bus */ > > path = qdict_get_try_str(opts, "bus"); > > if (path != NULL) { >
On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote: > On Thu, Oct 24, 2024 at 12:56:24PM -0400, Peter Xu wrote: > > Adding significant new functionality to QOM should really come > with a commit message explaining the rationale and the design > choices > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > > qom/object.c | 3 +++ > > qom/object_interfaces.c | 24 +++++++++++++++++ > > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > > system/qdev-monitor.c | 7 +++++ > > 5 files changed, 100 insertions(+), 3 deletions(-) > > snip > > > + * Singleton class describes the type of object classes that can only > > + * provide one instance for the whole lifecycle of QEMU. It will fail the > > + * operation if one attemps to create more than one instance. > > + * > > + * One can fetch the single object using class's get_instance() callback if > > + * it was created before. This can be useful for operations like QMP > > + * qom-list-properties, where dynamically creating an object might not be > > + * feasible. > > snip > > > +/** > > + * singleton_get_instance: > > + * > > + * @class: the class to fetch singleton instance > > + * > > + * Returns: the object* if the class is a singleton class and the singleton > > + * object is created, NULL otherwise. > > + */ > > +Object *singleton_get_instance(ObjectClass *class); > > With this design, all code that uses a given type needs to know > whether or not it is intended to be a singleton. If some code > somewhere mistakenly calls 'object_new' instead of 'singleton_get_instance', > the singleton type is no longer a singleton, except you handle this by > adding an assert in object_initialize_with_type. Yes, that's really the current goal, and why I added that assert(), so it fails any attempts trying to create one more instance of it, because normally it's by accident. The theory issue to solve is when some class is not ready for being created more than once, so it must not happen. My plan was to properly guard qdev creation with this which can fail pretty, so it's a "programming error" otherwise. > > This is still a bit of a loaded foot-gun IMHO, as we don't want random > code asserting. > > > diff --git a/qom/object.c b/qom/object.c > > index 11424cf471..ded299ae1a 100644 > > --- a/qom/object.c > > +++ b/qom/object.c > > @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type > > g_assert(type->abstract == false); > > g_assert(size >= type->instance_size); > > > > + /* Singleton class can only create one object */ > > + g_assert(!singleton_get_instance(type->class)); > > + > > memset(obj, 0, type->instance_size); > > obj->class = type->class; > > object_ref(obj); > > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > > index e91a235347..ecc1cf781c 100644 > > --- a/qom/qom-qmp-cmds.c > > +++ b/qom/qom-qmp-cmds.c > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > ObjectProperty *prop; > > ObjectPropertyIterator iter; > > ObjectPropertyInfoList *prop_list = NULL; > > + bool create; > > > > klass = module_object_class_by_name(typename); > > if (klass == NULL) { > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > return NULL; > > } > > > > - obj = object_new(typename); > > + /* Avoid creating multiple instances if the class is a singleton */ > > + create = !object_class_is_singleton(klass) || > > + !singleton_get_instance(klass); > > + > > + if (create) { > > + obj = object_new(typename); > > + } else { > > + obj = singleton_get_instance(klass); > > + } > > This is the first foot-gun example. > > I really strongly dislike that the design pattern forces us to > create code like this, as we can never be confident we've > correctly identified all the places which may call object_new > on a singleton... Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's, I'll comment below for that. Meanwhile I hope there should be very limited places in QEMU to randomly create "any" object on the fly.. so far only qom/device-list-properties that I see. > > > @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > QAPI_LIST_PREPEND(prop_list, info); > > } > > > > - object_unref(obj); > > + if (create) { > > + object_unref(obj); > > + } > > ...and this just compounds the ugliness. > > > @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, > > return NULL; > > } > > > > - if (object_class_is_abstract(klass)) { > > + /* > > + * Abstract classes are not for instantiations, meanwhile avoid > > + * creating temporary singleton objects because it can cause conflicts > > + * if there's already one created. > > + */ > > Another example of the foot-gun firing at random code > > > + if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) { > > object_class_property_iter_init(&iter, klass); > > } else { > > obj = object_new(typename); > > > With changes to QOM, I think it is generally informative to look at how > GLib has handled the problem, since the QOM design has heavily borrowed > from its GObject design. > > In GObject, singletons are handled in a very differnt way. It has a > concept of a "constructor" function against the class, which is what is > responsible for allocating the object. By default the 'constructor' will > call g_new0, but a class which wishes to become a singleton will override > the 'constructor' function to allocate on first call, and return the > cached object on subsequent calls. This is illustrated here: > > https://gitlab.gnome.org/GNOME/glib/-/blob/main/gobject/gobject.h#L297 > > The key benefit of this is that everything can carry on calling > g_object_new() as before, as it will just "do the right thing" > in terms of allocation. > > In QOM, we don't have a 'constructor' class function, we just directly > call g_malloc from object_new_with_type. This is because at the time, > we didn't see an immediate need for it. We could easily change that > though to introduce the concept of a 'constructor', which could > probably make singletons work without needing updates to existing code. I think glib's implementation is not thread safe on its own... consider two threads invoke g_object_new() on the singleton without proper locking. I am guessing it could be relevant to glib's heavy event model. And that's fundamentally what I want to have for QEMU's migration object, so that it doesn't need locking on its own of the singleton idea: the locking, if necessary, should be done in get_instance(), in this case. So I did it wrong indeed in this current series at least there, where it should need to take migration's new mutex, then take a refcount before return, rightfully as Markus pointed out elsewhere. The other concern I have with glib's singleton is, fundamentally object_new() didn't really create a new object, which can be against the gut feelings of whoever read it. I'm not sure whether that's really what we want.. or maybe that's only my own concern, so it might be subjective. Thanks,
On Fri, Oct 25, 2024 at 12:11:51PM -0300, Philippe Mathieu-Daudé wrote: > On 24/10/24 17:53, Peter Xu wrote: > > On Thu, Oct 24, 2024 at 05:02:19PM -0300, Philippe Mathieu-Daudé wrote: > > > Hi Peter, > > > > Hi, Phil, > > > > Thanks for the quick reviews! > > > > > > > > (Cc'ing Mark) > > > > > > On 24/10/24 13:56, Peter Xu wrote: > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ > > > > qom/object.c | 3 +++ > > > > qom/object_interfaces.c | 24 +++++++++++++++++ > > > > qom/qom-qmp-cmds.c | 22 ++++++++++++--- > > > > system/qdev-monitor.c | 7 +++++ > > > > 5 files changed, 100 insertions(+), 3 deletions(-) > > > > > > > > > > +/** > > > > + * SingletonClass: > > > > + * > > > > + * @parent_class: the base class > > > > + * @get_instance: fetch the singleton instance if it is created, > > > > + * NULL otherwise. > > > > + * > > > > + * Singleton class describes the type of object classes that can only > > > > + * provide one instance for the whole lifecycle of QEMU. It will fail the > > > > + * operation if one attemps to create more than one instance. > > > > + * > > > > + * One can fetch the single object using class's get_instance() callback if > > > > + * it was created before. This can be useful for operations like QMP > > > > + * qom-list-properties, where dynamically creating an object might not be > > > > + * feasible. > > > > + */ > > > > +struct SingletonClass { > > > > + /* <private> */ > > > > + InterfaceClass parent_class; > > > > + /* <public> */ > > > > + Object *(*get_instance)(Error **errp); > > > > > > IMHO asking get_instance() is overkill ... > > > > > > > +}; > > > > + > > > > +/** > > > > + * object_class_is_singleton: > > > > + * > > > > + * @class: the class to detect singleton > > > > + * > > > > + * Returns: true if it's a singleton class, false otherwise. > > > > + */ > > > > +bool object_class_is_singleton(ObjectClass *class); > > > > + > > > > +/** > > > > + * singleton_get_instance: > > > > + * > > > > + * @class: the class to fetch singleton instance > > > > + * > > > > + * Returns: the object* if the class is a singleton class and the singleton > > > > + * object is created, NULL otherwise. > > > > + */ > > > > +Object *singleton_get_instance(ObjectClass *class); > > > > + > > > > #endif > > > > > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > > > > index e0833c8bfe..6766060d0a 100644 > > > > --- a/qom/object_interfaces.c > > > > +++ b/qom/object_interfaces.c > > > > @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) > > > > object_unparent(object_get_objects_root()); > > > > } > > > > +bool object_class_is_singleton(ObjectClass *class) > > > > +{ > > > > + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); > > > > +} > > > > + > > > > +Object *singleton_get_instance(ObjectClass *class) > > > > +{ > > > > > > ... when we can use object_resolve_type_unambiguous: > > > > > > return object_resolve_type_unambiguous(object_class_get_name(class, > > > NULL)); > > > > I think an issue is migration object is nowhere to be find under > > object_get_root(), so it won't work there. A side benefit is, it's also > > faster.. > > Maybe a simpler alternative is to add a field in ObjectClass, maintain > a single GHashTable to store TypeName -> Instance and retrieve as: > > Object *singleton_get_instance(ObjectClass *class) > { > return g_hash_table_lookup(&singletons, > object_class_get_name(class)); > } This may need some proper locking too as Markus pointed out, so that the object needs to boost its refcount before returned. It might be a problem if the singleton class has its own locking.. I'll need to think about it. In migration's case, there's going to be a new migration mutex protecting the singleton object being updated (not included in this series, posted elsewhere [1]). [1] https://lore.kernel.org/r/20241024213056.1395400-9-peterx@redhat.com > > TBH the TYPE_SINGLETON interface seems a bit over-engineered and its > implementation error prone. Please feel free to suggest something else if you come up with. The issue so far is qom/device-list-properties now can randomly create migration object, which is defined to be global, while the plan is we need to do something tricky in instance_finalize() for migration object (operate on global vars; which is tricky but it could solve some issue we encounter), so we must make sure it's never created more than once. Thanks, > > > How about I use object_resolve_type_unambiguous() as a fallback? Then it's > > used only if get_instance() is not provided. > > > > > > > > BTW should we pass Error** argument to singleton_get_instance()? > > > > I didn't expect it to fail, hence I didn't even add it to make it more like > > "this will always success or it asserts" kind of things. I left Error** in > > the hook just to be slightly flexible, but I always pass in error_abort() > > in this patch. > > > > I can either add Error** if anyone thinks it useful, or even drop Error** > > in ->get_instance() since it's mostly not used at least for now. > > > > Let me know! > > > > > > > > > + SingletonClass *singleton = > > > > + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); > > > > + > > > > + if (!singleton) { > > > > + return NULL; > > > > + } > > > > + > > > > + return singleton->get_instance(&error_abort); > > > > +} > > > > > > Alternatively call object_resolve_type_unambiguous() in instance_init()? > > > > Thanks, > > >
On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote: > On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote: > > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > > > index e91a235347..ecc1cf781c 100644 > > > --- a/qom/qom-qmp-cmds.c > > > +++ b/qom/qom-qmp-cmds.c > > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > > ObjectProperty *prop; > > > ObjectPropertyIterator iter; > > > ObjectPropertyInfoList *prop_list = NULL; > > > + bool create; > > > > > > klass = module_object_class_by_name(typename); > > > if (klass == NULL) { > > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > > return NULL; > > > } > > > > > > - obj = object_new(typename); > > > + /* Avoid creating multiple instances if the class is a singleton */ > > > + create = !object_class_is_singleton(klass) || > > > + !singleton_get_instance(klass); > > > + > > > + if (create) { > > > + obj = object_new(typename); > > > + } else { > > > + obj = singleton_get_instance(klass); > > > + } > > > > This is the first foot-gun example. > > > > I really strongly dislike that the design pattern forces us to > > create code like this, as we can never be confident we've > > correctly identified all the places which may call object_new > > on a singleton... > > Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's, > I'll comment below for that. > > Meanwhile I hope there should be very limited places in QEMU to randomly > create "any" object on the fly.. so far only qom/device-list-properties > that I see. There's -object/-device CLI, and their corresponding object_add / device_add commands. They are not relevant for the migration object, but you're adding this feature to QOM, so we have to take them into account. If anyone defines another Object or Device class as a singleton, we will have quite a few places where we can trigger the assert. This is especially bad if we trigger it from anything in QMP as that kills a running guest. > > I think glib's implementation is not thread safe on its own... consider two > threads invoke g_object_new() on the singleton without proper locking. I > am guessing it could be relevant to glib's heavy event model. I've not checked the full code path, to see if they have a serialization point somewhere it the g_object_new call chain, but yes, it is a valid concern that would need to be resolved. With regards, Daniel
On Fri, Oct 25, 2024, 5:51 a.m. Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Oct 24, 2024 at 12:56:24PM -0400, Peter Xu wrote: > > Adding significant new functionality to QOM should really come > with a commit message explaining the rationale and the design > choices > Ohh i definitely missed it, my bad. Luckily i still wrote the cover letter. I'll prepare that if there is v2. >
On Fri, Oct 25, 2024 at 05:22:01PM +0100, Daniel P. Berrangé wrote: > On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote: > > On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote: > > > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > > > > index e91a235347..ecc1cf781c 100644 > > > > --- a/qom/qom-qmp-cmds.c > > > > +++ b/qom/qom-qmp-cmds.c > > > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > > > ObjectProperty *prop; > > > > ObjectPropertyIterator iter; > > > > ObjectPropertyInfoList *prop_list = NULL; > > > > + bool create; > > > > > > > > klass = module_object_class_by_name(typename); > > > > if (klass == NULL) { > > > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > > > return NULL; > > > > } > > > > > > > > - obj = object_new(typename); > > > > + /* Avoid creating multiple instances if the class is a singleton */ > > > > + create = !object_class_is_singleton(klass) || > > > > + !singleton_get_instance(klass); > > > > + > > > > + if (create) { > > > > + obj = object_new(typename); > > > > + } else { > > > > + obj = singleton_get_instance(klass); > > > > + } > > > > > > This is the first foot-gun example. > > > > > > I really strongly dislike that the design pattern forces us to > > > create code like this, as we can never be confident we've > > > correctly identified all the places which may call object_new > > > on a singleton... > > > > Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's, > > I'll comment below for that. > > > > Meanwhile I hope there should be very limited places in QEMU to randomly > > create "any" object on the fly.. so far only qom/device-list-properties > > that I see. > > There's -object/-device CLI, and their corresponding object_add > / device_add commands. Ah I didn't mention that when reply, but this patch blocks properly any device-add for singleton objects for more than once. Please see the change in qdev_device_add_from_qdict(). For migration object, it means it'll always fail because migration object is created very early, before someone can try to create it. Not to mention it also has dc->hotpluggable==false, so things like -device will never work on migration object. For object-add, IIUC migration object should always be safe because it has no USER_CREATABLE interface. > > They are not relevant for the migration object, but you're adding > this feature to QOM, so we have to take them into account. If anyone > defines another Object or Device class as a singleton, we will have > quite a few places where we can trigger the assert. This is especially > bad if we trigger it from anything in QMP as that kills a running > guest. > > > > > I think glib's implementation is not thread safe on its own... consider two > > threads invoke g_object_new() on the singleton without proper locking. I > > am guessing it could be relevant to glib's heavy event model. > > I've not checked the full code path, to see if they have a serialization > point somewhere it the g_object_new call chain, but yes, it is a valid > concern that would need to be resolved. Thanks,
On Fri, Oct 25, 2024 at 06:10:46PM -0400, Peter Xu wrote: > On Fri, Oct 25, 2024 at 05:22:01PM +0100, Daniel P. Berrangé wrote: > > On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote: > > > On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote: > > > > > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c > > > > > index e91a235347..ecc1cf781c 100644 > > > > > --- a/qom/qom-qmp-cmds.c > > > > > +++ b/qom/qom-qmp-cmds.c > > > > > @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > > > > ObjectProperty *prop; > > > > > ObjectPropertyIterator iter; > > > > > ObjectPropertyInfoList *prop_list = NULL; > > > > > + bool create; > > > > > > > > > > klass = module_object_class_by_name(typename); > > > > > if (klass == NULL) { > > > > > @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, > > > > > return NULL; > > > > > } > > > > > > > > > > - obj = object_new(typename); > > > > > + /* Avoid creating multiple instances if the class is a singleton */ > > > > > + create = !object_class_is_singleton(klass) || > > > > > + !singleton_get_instance(klass); > > > > > + > > > > > + if (create) { > > > > > + obj = object_new(typename); > > > > > + } else { > > > > > + obj = singleton_get_instance(klass); > > > > > + } > > > > > > > > This is the first foot-gun example. > > > > > > > > I really strongly dislike that the design pattern forces us to > > > > create code like this, as we can never be confident we've > > > > correctly identified all the places which may call object_new > > > > on a singleton... > > > > > > Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's, > > > I'll comment below for that. > > > > > > Meanwhile I hope there should be very limited places in QEMU to randomly > > > create "any" object on the fly.. so far only qom/device-list-properties > > > that I see. > > > > There's -object/-device CLI, and their corresponding object_add > > / device_add commands. > > Ah I didn't mention that when reply, but this patch blocks properly any > device-add for singleton objects for more than once. Please see the change > in qdev_device_add_from_qdict(). > > For migration object, it means it'll always fail because migration object > is created very early, before someone can try to create it. Not to mention > it also has dc->hotpluggable==false, so things like -device will never work > on migration object. > > For object-add, IIUC migration object should always be safe because it has > no USER_CREATABLE interface. > > > > > They are not relevant for the migration object, but you're adding > > this feature to QOM, so we have to take them into account. If anyone > > defines another Object or Device class as a singleton, we will have > > quite a few places where we can trigger the assert. This is especially > > bad if we trigger it from anything in QMP as that kills a running > > guest. Sorry, for some reason I think I didn't notice the 2nd paragraph.. And somehow I was only thinking the migration use case. Indeed this part is not easy to get right. I hope this is not a blocker yet so far. We can have a look when I send the new version; I'll start to mark the series RFC. Meanwhile I'll have a closer look, hopefully object_class_is_abstract() is a good point of reference where I can track most of dynamic creations of objects, and I'll go from there. My current plan is we can have one helper object_new_allowed(), where it should contain both the check over abstract & singleton classes before any instantiations happen.
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index 02b11a7ef0..9b2cc0e554 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -177,4 +177,51 @@ bool user_creatable_del(const char *id, Error **errp); */ void user_creatable_cleanup(void); +#define TYPE_SINGLETON "singleton" + +typedef struct SingletonClass SingletonClass; +DECLARE_CLASS_CHECKERS(SingletonClass, SINGLETON, TYPE_SINGLETON) + +/** + * SingletonClass: + * + * @parent_class: the base class + * @get_instance: fetch the singleton instance if it is created, + * NULL otherwise. + * + * Singleton class describes the type of object classes that can only + * provide one instance for the whole lifecycle of QEMU. It will fail the + * operation if one attemps to create more than one instance. + * + * One can fetch the single object using class's get_instance() callback if + * it was created before. This can be useful for operations like QMP + * qom-list-properties, where dynamically creating an object might not be + * feasible. + */ +struct SingletonClass { + /* <private> */ + InterfaceClass parent_class; + /* <public> */ + Object *(*get_instance)(Error **errp); +}; + +/** + * object_class_is_singleton: + * + * @class: the class to detect singleton + * + * Returns: true if it's a singleton class, false otherwise. + */ +bool object_class_is_singleton(ObjectClass *class); + +/** + * singleton_get_instance: + * + * @class: the class to fetch singleton instance + * + * Returns: the object* if the class is a singleton class and the singleton + * object is created, NULL otherwise. + */ +Object *singleton_get_instance(ObjectClass *class); + #endif diff --git a/qom/object.c b/qom/object.c index 11424cf471..ded299ae1a 100644 --- a/qom/object.c +++ b/qom/object.c @@ -553,6 +553,9 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type g_assert(type->abstract == false); g_assert(size >= type->instance_size); + /* Singleton class can only create one object */ + g_assert(!singleton_get_instance(type->class)); + memset(obj, 0, type->instance_size); obj->class = type->class; object_ref(obj); diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index e0833c8bfe..6766060d0a 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -354,6 +354,23 @@ void user_creatable_cleanup(void) object_unparent(object_get_objects_root()); } +bool object_class_is_singleton(ObjectClass *class) +{ + return !!object_class_dynamic_cast(class, TYPE_SINGLETON); +} + +Object *singleton_get_instance(ObjectClass *class) +{ + SingletonClass *singleton = + (SingletonClass *)object_class_dynamic_cast(class, TYPE_SINGLETON); + + if (!singleton) { + return NULL; + } + + return singleton->get_instance(&error_abort); +} + static void register_types(void) { static const TypeInfo uc_interface_info = { @@ -362,7 +379,14 @@ static void register_types(void) .class_size = sizeof(UserCreatableClass), }; + static const TypeInfo singleton_interface_info = { + .name = TYPE_SINGLETON, + .parent = TYPE_INTERFACE, + .class_size = sizeof(SingletonClass), + }; + type_register_static(&uc_interface_info); + type_register_static(&singleton_interface_info); } type_init(register_types) diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c index e91a235347..ecc1cf781c 100644 --- a/qom/qom-qmp-cmds.c +++ b/qom/qom-qmp-cmds.c @@ -126,6 +126,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, ObjectProperty *prop; ObjectPropertyIterator iter; ObjectPropertyInfoList *prop_list = NULL; + bool create; klass = module_object_class_by_name(typename); if (klass == NULL) { @@ -141,7 +142,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, return NULL; } - obj = object_new(typename); + /* Avoid creating multiple instances if the class is a singleton */ + create = !object_class_is_singleton(klass) || + !singleton_get_instance(klass); + + if (create) { + obj = object_new(typename); + } else { + obj = singleton_get_instance(klass); + } object_property_iter_init(&iter, obj); while ((prop = object_property_iter_next(&iter))) { @@ -172,7 +181,9 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename, QAPI_LIST_PREPEND(prop_list, info); } - object_unref(obj); + if (create) { + object_unref(obj); + } return prop_list; } @@ -199,7 +210,12 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename, return NULL; } - if (object_class_is_abstract(klass)) { + /* + * Abstract classes are not for instantiations, meanwhile avoid + * creating temporary singleton objects because it can cause conflicts + * if there's already one created. + */ + if (object_class_is_abstract(klass) || object_class_is_singleton(klass)) { object_class_property_iter_init(&iter, klass); } else { obj = object_new(typename); diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index 44994ea0e1..1310f35c9f 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -36,6 +36,7 @@ #include "qemu/option.h" #include "qemu/qemu-print.h" #include "qemu/option_int.h" +#include "qom/object_interfaces.h" #include "sysemu/block-backend.h" #include "migration/misc.h" #include "qemu/cutils.h" @@ -643,6 +644,12 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, return NULL; } + if (singleton_get_instance(OBJECT_CLASS(dc))) { + error_setg(errp, "Class '%s' only supports one instance", + driver); + return NULL; + } + /* find bus */ path = qdict_get_try_str(opts, "bus"); if (path != NULL) {
Signed-off-by: Peter Xu <peterx@redhat.com> --- include/qom/object_interfaces.h | 47 +++++++++++++++++++++++++++++++++ qom/object.c | 3 +++ qom/object_interfaces.c | 24 +++++++++++++++++ qom/qom-qmp-cmds.c | 22 ++++++++++++--- system/qdev-monitor.c | 7 +++++ 5 files changed, 100 insertions(+), 3 deletions(-)