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
