On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote:
> 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 <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(-)
>
> 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.
Yes, that's really the current goal, and why I added that assert(), so it
fails any attempts trying to create one more instance of it, because
normally it's by accident. The theory issue to solve is when some class is
not ready for being created more than once, so it must not happen.
My plan was to properly guard qdev creation with this which can fail
pretty, so it's a "programming error" otherwise.
>
> 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...
Yeah I agree it's not pretty, IMHO it's a trade-off comparing to glib's,
I'll comment below for that.
Meanwhile I hope there should be very limited places in QEMU to randomly
create "any" object on the fly.. so far only qom/device-list-properties
that I see.
>
> > @@ -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.
I think glib's implementation is not thread safe on its own... consider two
threads invoke g_object_new() on the singleton without proper locking. I
am guessing it could be relevant to glib's heavy event model.
And that's fundamentally what I want to have for QEMU's migration object,
so that it doesn't need locking on its own of the singleton idea: the
locking, if necessary, should be done in get_instance(), in this case.
So I did it wrong indeed in this current series at least there, where it
should need to take migration's new mutex, then take a refcount before
return, rightfully as Markus pointed out elsewhere.
The other concern I have with glib's singleton is, fundamentally
object_new() didn't really create a new object, which can be against the
gut feelings of whoever read it. I'm not sure whether that's really what
we want.. or maybe that's only my own concern, so it might be subjective.
Thanks,
--
Peter Xu
