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

[Daniel P . Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|