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

[Peter Xu <peterx@redhat.com>]

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.

The real extra "complexity" is object_new_or_fetch(), but I hope it's not a
major concern either.  We only have two such use (aka, "please give me an
object of class XXX"), which is qom/device-list-properties.  I don't expect
it to be common, I hope it's easy to maintain.

> 
> The conversion of the iommu code results in worse error reporting, and
> doesn't handle the virtio-iommu case, and the migration problems appear
> solvable without inventing a singleton interface. So this doesn't feel
> like it is worth the the trouble.

IMHO that's not a major issue, I can drop patch 3-5 just to make it simple
as of now.  Btw, I have a TODO in patch 2 where I mentioned we can provide
better error report if we want, so we can easily have exactly the same
error as before with maybe a few or 10+ LOCs on top.  It's trivial.

object_new_allowed():

+    if (object_class_is_singleton(klass)) {
+        Object *obj = singleton_get_instance(klass);
+
+        if (obj) {
+            object_unref(obj);
+            /*
+             * TODO: Enhance the error message.  E.g., the singleton class
+             * can provide a per-class error message in SingletonClass.
+             */
+            error_setg(errp, "Object type '%s' conflicts with "
+                       "an existing singleton instance",
+                       klass->type->name);
+            return false;
+        }
+    }

> 
> 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.

Thanks,

-- 
Peter Xu