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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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