Re: [Qemu-devel] [PATCH v4 7/8] qmp: Support abstract classes on device-list-properties

[Eduardo Habkost]

On Mon, Nov 07, 2016 at 05:41:01PM +0000, Daniel P. Berrange wrote:
> On Mon, Nov 07, 2016 at 03:27:31PM -0200, Eduardo Habkost wrote:
> > On Mon, Nov 07, 2016 at 04:51:57PM +0100, Markus Armbruster wrote:
> > > "Daniel P. Berrange" <address@hidden> writes:
> > > 
> > > > On Mon, Nov 07, 2016 at 03:48:49PM +0100, Halil Pasic wrote:
> > > >> 
> > > >> 
> > > >> On 11/07/2016 02:05 PM, Eduardo Habkost wrote:
> > > >> > If you want some subclasses to not have the property, then I
> > > >> > recommend not registering it as a class property on the base
> > > >> > class in the first place. I don't expect to see a mechanism to
> > > >> > allow subclasses to remove or override class properties from
> > > >> > parent classes.
> > > >> > 
> > > >> 
> > > >> Thank you very much for your reply.
> > > >> 
> > > >> I understand, yet I see potential problems. The example with ioeventfd
> > > >> and vhost in virtio-pci is a good one also because  the first there was
> > > >> the ioeventfd property with commit 653ced07 and then the vhost case 
> > > >> came
> > > >> along with commit 50787628ee3 (ok ioeventfd is not there for some non
> > > >> vhost virtio-pci devices for reasons I do not understand).
> > > >> 
> > > >> To rephrase this in generic context a specialization for which a
> > > >> property does not make sense might come along after the property at the
> > > >> base class was established.
> > > >> 
> > > >> Now AFAIU properties are external API, so having to make a 
> > > >> compatibility
> > > >> breaking change there might not be fun. Does this mean one should be
> > > >> very careful to put only use class level properties on abstract classes
> > > >> where its certain that the property always makes sense including it's
> > > >> access control?
> > > >
> > > > This could be an argument for *NOT* allowing introspectiing of 
> > > > properties
> > > > against abstract parent classes. If you only ever allow introspecting 
> > > > against
> > > > leaf node non-abstract classes, then QEMU retains the freedom to move 
> > > > props
> > > > from a base class down to an leaf class without risk of breaking mgmt 
> > > > apps.
> > > 
> > > That's a really good point.  To generalize it a bit, introspection of
> > > actual interfaces is fine, but permitting introspection of how they are
> > > made can add artificial constraints.
> > > 
> > > Introspecting the subtype relation is already problematic in this view.
> > 
> > Yes, that's a very good point. But note that that this means
> > making things more complex for libvirt.
> > 
> > In the case of -cpu, if we don't expose (or allow libvirt to
> > making assumptions about) subtype relations, the only way libvirt
> > can conclude that "+foo can be used as -cpu option with any CPU
> > model", is to query each and every CPU model type, and see if all
> > of them support the "foo" property.
> >
> > It's a trade-off between an interface that's more complex to use
> > and having less freedom to change the class hierarchy.
> > Personally, I don't mind going either way, if we have a good
> > reason for that.
> 
> Or could do a tradeoff where we allow introspection of abstract
> parent classes, but explicitly document that we reserve the right
> to move properties to leaf nodes ?

Reserving the right to move properties to leaf nodes would be
welcome. But it would force libvirt to query all leaf nodes if it
wants to be sure the option is really unsupported by the QEMU
binary, so why would libvirt query the parent class in the first
place?

-- 
Eduardo