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.
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.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
