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

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 25, 2024 at 06:10:46PM -0400, Peter Xu wrote:
> 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.

Sorry, for some reason I think I didn't notice the 2nd paragraph.. And
somehow I was only thinking the migration use case.

Indeed this part is not easy to get right.  I hope this is not a blocker
yet so far.  We can have a look when I send the new version; I'll start to
mark the series RFC.

Meanwhile I'll have a closer look, hopefully object_class_is_abstract() is
a good point of reference where I can track most of dynamic creations of
objects, and I'll go from there.

My current plan is we can have one helper object_new_allowed(), where it
should contain both the check over abstract & singleton classes before any
instantiations happen.

-- 
Peter Xu