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

[Eduardo Habkost]

On Tue, Nov 08, 2016 at 03:35:08PM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> writes:
> 
> > On Tue, Nov 08, 2016 at 08:37:20AM +0100, Markus Armbruster wrote:
> >> Eduardo Habkost <address@hidden> writes:
> >> 
> >> > On Mon, Nov 07, 2016 at 06:08:42PM +0000, Daniel P. Berrange wrote:
> >> >> On Mon, Nov 07, 2016 at 04:03:58PM -0200, Eduardo Habkost wrote:
> >> >> > 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?
> >> >> 
> >> >> The introspection API is quite general purpose so its semantics have to
> >> >> be suitable for all types of object, but some types of object may not 
> >> >> need
> >> >> the full degree of flexibility. So what I meant was that while we want
> >> >> to be able to move props down to leaf classes for objects in general,
> >> >> we could perhaps assume that this will never happen for CPU model 
> >> >> objects.
> >> >
> >> > This would work for me. I only worry that any code that makes the
> >> > wrong assumptions (on either QEMU or libvirt) would easily go
> >> > unnoticed until we try to change the class hierarchy and it
> >> > breaks something.
> >> >
> >> > Markus, what do you think?
> >> 
> >> I dislike complexity in interface contracts.
> >> 
> >> Guidance like "if you want to learn the properties of a type T,
> >> introspect T" is simple.
> >> 
> >> Guidance like "if you want to learn the properties common to all
> >> subtypes of T, you need to introspect all subtypes of T, except when T
> >> is "cpu", you can take a shortcut and introspect T instead" is not
> >> simple, and the precedent opens the gates to even more complexity.
> >> 
> >> Is introspecting all CPU types of interest really that bad?
> >
> > I'm no sure - adding Jiri who'll ultimately be writing this code ?
> >
> > If we have to introspect M cpu flags * N cpu models, this will get slow
> > very quickly as IIRC there's 100+ cpu flags, and 10+ models, so 1000+
> > combinations
> 
> for CPU in CPUs
>     if this is the first one
>         common_flags = CPU's flags
>     else
>         common_flags &= CPU's flags
> 
> Yes, that's O(M*N), but the I/O is O(N): you query for each CPU just
> once.  I'd expect I/O to dominate even with hundreds of CPU flags.
> 
> In general, if a management application introspects the same things via
> QMP multiple times, it's probably doing it wrong.

Or they could query only the CPU model that is being used for a
VM, when validating the VM configuration. But I'm not sure when
exactly this information is going to be used by libvirt.

-- 
Eduardo