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

[Peter Xu <peterx@redhat.com>]

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,
> > 
> 

-- 
Peter Xu