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

[Halil Pasic]

On 11/08/2016 08:37 AM, 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.
> 

+1

> Is introspecting all CPU types of interest really that bad?
> 

A simple to use interface could eventually also be achieved by introducing
operations for these things which currently seem cumbersome with 
(e.g. properties common to all subtypes of T) the current set of (primitive)
operations. This would also leave us more degrees of freedom regarding
implementation details and also other stuff. (Sorry if this was
already discussed with previous versions of the patchset).

Regards,
Halil