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
