From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34138) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c47Ua-0007xZ-V4 for qemu-devel@nongnu.org; Tue, 08 Nov 2016 09:35:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c47UW-0004GN-1F for qemu-devel@nongnu.org; Tue, 08 Nov 2016 09:35:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59460) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c47UV-0004GC-PQ for qemu-devel@nongnu.org; Tue, 08 Nov 2016 09:35:11 -0500 From: Markus Armbruster References: <20161107130556.GQ5057@thinpad.lan.raisama.net> <9ad4d662-df82-06d0-52a2-ec92b34b9733@linux.vnet.ibm.com> <20161107150153.GB15102@redhat.com> <87oa1rp9sy.fsf@dusky.pond.sub.org> <20161107172731.GS5057@thinpad.lan.raisama.net> <20161107174101.GD19422@redhat.com> <20161107180358.GT5057@thinpad.lan.raisama.net> <20161107180841.GE19422@redhat.com> <20161107182808.GU5057@thinpad.lan.raisama.net> <8760nymngv.fsf@dusky.pond.sub.org> <20161108100947.GF14865@redhat.com> Date: Tue, 08 Nov 2016 15:35:08 +0100 In-Reply-To: <20161108100947.GF14865@redhat.com> (Daniel P. Berrange's message of "Tue, 8 Nov 2016 10:09:47 +0000") Message-ID: <87y40uf3ab.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 7/8] qmp: Support abstract classes on device-list-properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Jiri Denemark , Igor Mammedov , Halil Pasic , Eduardo Habkost , Andreas =?utf-8?Q?F=C3=A4rber?= , qemu-devel@nongnu.org "Daniel P. Berrange" writes: > On Tue, Nov 08, 2016 at 08:37:20AM +0100, Markus Armbruster wrote: >> Eduardo Habkost 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" 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.