On Tue, Nov 08, 2016 at 08:37:20AM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabk...@redhat.com> 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" <berra...@redhat.com> 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 Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|