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