On Wed, Oct 30, 2024 at 03:08:08PM -0400, Peter Xu wrote: > On Wed, Oct 30, 2024 at 06:07:08PM +0000, Daniel P. Berrangé wrote: > > On Tue, Oct 29, 2024 at 05:16:00PM -0400, Peter Xu wrote: > > > v1: https://lore.kernel.org/r/20241024165627.1372621-1-pet...@redhat.com > > > > > > This patchset introduces the singleton interface for QOM. I didn't add a > > > changelog because there're quite a few changes here and there, plus new > > > patches. So it might just be easier to re-read, considering the patchset > > > isn't large. > > > > > > I switched v2 into RFC, because we have reviewer concerns (Phil and Dan so > > > far) that it could be error prone to try to trap every attempts to create > > > an object. My argument is, if we already have abstract class, meanwhile > > > we > > > do not allow instantiation of abstract class, so the complexity is already > > > there. I prepared patch 1 this time to collect and track all similar > > > random object creations; it might be helpful as a cleanup on its own to > > > deduplicate some similar error messages. Said that, I'm still always open > > > to rejections to this proposal. > > > > > > I hope v2 looks slightly cleaner by having not only object_new_allowed() > > > but also object_new_or_fetch(). > > > > For me, that doesn't really make it much more appealing. Yes, we already > > have > > an abstract class, but that has narrower impact, as there are fewer places > > in which which we can trigger instantiation of an abstract class, than > > where we can trigger instantiation of arbitrary objects and devices. > > There should be exactly the same number of places that will need care for > either abstract or singleton. I tried to justify this with patch 1. > > I still think patch 1 can be seen as a cleanup too on its own (dedups the > same "The class is abstract" error message), tracking random object > creations so logically we could have the idea on whether a class can be > instantiated at all, starting with abstract class.
I think patch 1 might be incomplete, as I'm not seeing what checks for abstract or singleton classes in the 'qdev_new' code paths, used by -device / device_add QMP. This is an example of the risks of adding more failure scenarios to object_add. > > NB, my view point would have been different if 'object_new' had an > > "Error *errp" parameter. That would have made handling failure a > > standard part of the design pattern for object construction, thus > > avoiding adding asserts in the 'object_new' codepath which could be > > triggered by unexpected/badly validated user input. > > Yes I also wished object_new() can take an Error** when I started working > on it. It would make this much easier, indeed. I suppose we don't need > that by not allowing instance_init() to fail at all, postponing things to > realize(). I suppose that's a "tactic" QEMU chose explicitly to make it > easy that object_new() callers keep like before with zero error handling > needed. At least for TYPE_DEVICE it looks all fine if all such operations > can be offloaded into realize(). I'm not sure user creatable has those > steps also because of this limitation. > > I was trying to do that with object_new_allowed() here instead, whenever it > could be triggered by an user input. We could have an extra layer before > reaching object_new() to guard any user input, and I think > object_new_allowed() could play that role. When / If we want to introduce > Error** to object_new() some day (or a variance of it), we could simply > move object_new_allowed() into it. Yes, having thought about this today, I came up with a way that we could introduce a object_new_dynamic() variant with "Error *errp" instead of asserts, and *crucially* force its use in the unsafe scenarios. ie any place that is not passing a const,static string. I've CC'd you on an RFC series that mocks up this idea. That would be sufficient to remove my objections wrt the singleton concept. 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 :|
