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

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 25, 2024 at 05:22:01PM +0100, Daniel P. Berrangé wrote:
> On Fri, Oct 25, 2024 at 12:17:02PM -0400, Peter Xu wrote:
> > On Fri, Oct 25, 2024 at 10:51:21AM +0100, Daniel P. Berrangé wrote:
> > > > 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.
> 
> There's -object/-device CLI, and their corresponding object_add
> / device_add commands.

Ah I didn't mention that when reply, but this patch blocks properly any
device-add for singleton objects for more than once.  Please see the change
in qdev_device_add_from_qdict().

For migration object, it means it'll always fail because migration object
is created very early, before someone can try to create it.  Not to mention
it also has dc->hotpluggable==false, so things like -device will never work
on migration object.

For object-add, IIUC migration object should always be safe because it has
no USER_CREATABLE interface.

> 
> They are not relevant for the migration object, but you're adding
> this feature to QOM, so we have to take them into account. If anyone
> defines another Object or Device class as a singleton, we will have
> quite a few places where we can trigger the assert. This is especially
> bad if we trigger it from anything in QMP as that kills a running
> guest.
> 
> > 
> > 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.
> 
> I've not checked the full code path, to see if they have a serialization
> point somewhere it the g_object_new call chain, but yes, it is a valid
> concern that would need to be resolved.

Thanks,

-- 
Peter Xu