Re: [PATCH RFC v2 0/7] QOM: Singleton interface

["Daniel P. Berrangé" <berrange@redhat.com>]

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-peterx@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 :|