Re: [PATCH 1/4] qom: TYPE_SINGLETON interface

[Peter Xu <peterx@redhat.com>]

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) {
> 

-- 
Peter Xu