On Fri, Oct 25, 2024 at 10:07:59AM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > Signed-off-by: Peter Xu <pet...@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) {
>
--
Peter Xu
