From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org, "Fabiano Rosas" <farosas@suse.de>,
"Igor Mammedov" <imammedo@redhat.com>,
"Juraj Marcin" <jmarcin@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
"Dr . David Alan Gilbert" <dave@treblig.org>,
"Cédric Le Goater" <clg@redhat.com>,
"Markus Armbruster" <armbru@redhat.com>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Alex Williamson" <alex.williamson@redhat.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Peter Maydell" <peter.maydell@linaro.org>
Subject: Re: [PATCH 1/4] qom: TYPE_SINGLETON interface
Date: Fri, 25 Oct 2024 17:22:01 +0100 [thread overview]
Message-ID: <ZxvFqYASUdSNtlr6@redhat.com> (raw)
In-Reply-To: <ZxvEfrj5B59J5HHj@x1n>
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 :|
next prev parent reply other threads:[~2024-10-25 16:22 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-24 16:56 [PATCH 0/4] QOM: Singleton interface Peter Xu
2024-10-24 16:56 ` [PATCH 1/4] qom: TYPE_SINGLETON interface Peter Xu
2024-10-24 20:02 ` Philippe Mathieu-Daudé
2024-10-24 20:53 ` Peter Xu
2024-10-25 15:11 ` Philippe Mathieu-Daudé
2024-10-25 16:21 ` Peter Xu
2024-10-25 8:07 ` Markus Armbruster
2024-10-25 15:17 ` Peter Xu
2024-10-25 9:51 ` Daniel P. Berrangé
2024-10-25 16:17 ` Peter Xu
2024-10-25 16:22 ` Daniel P. Berrangé [this message]
2024-10-25 22:10 ` Peter Xu
2024-10-29 0:01 ` Peter Xu
2024-10-25 16:37 ` Peter Xu
2024-10-24 16:56 ` [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object Peter Xu
2024-10-25 9:25 ` Markus Armbruster
2024-10-25 21:55 ` Peter Xu
2024-10-25 22:13 ` Peter Xu
2024-11-07 11:12 ` Markus Armbruster
2024-11-07 15:29 ` Peter Xu
2024-11-08 8:50 ` Markus Armbruster
2024-10-29 10:47 ` Daniel P. Berrangé
2024-10-29 14:32 ` Peter Xu
2024-10-24 16:56 ` [PATCH 3/4] migration: Make migration object " Peter Xu
2024-10-24 19:20 ` Fabiano Rosas
2024-10-24 16:56 ` [PATCH 4/4] migration: Reset current_migration properly Peter Xu
2024-10-24 19:34 ` Fabiano Rosas
2024-10-24 20:15 ` Peter Xu
2024-10-24 20:51 ` Fabiano Rosas
2024-10-25 7:38 ` [PATCH 0/4] QOM: Singleton interface Markus Armbruster
2024-10-25 15:01 ` Peter Xu
2024-10-29 10:42 ` Daniel P. Berrangé
2024-10-29 14:45 ` Peter Xu
2024-10-29 16:04 ` Daniel P. Berrangé
2024-10-29 17:05 ` Peter Xu
2024-10-29 17:17 ` Daniel P. Berrangé
2024-12-11 8:19 ` Markus Armbruster
2024-12-11 22:10 ` Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZxvFqYASUdSNtlr6@redhat.com \
--to=berrange@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=armbru@redhat.com \
--cc=clg@redhat.com \
--cc=dave@treblig.org \
--cc=eduardo@habkost.net \
--cc=farosas@suse.de \
--cc=imammedo@redhat.com \
--cc=jmarcin@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).