From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dX9z1-0004bs-Pr for qemu-devel@nongnu.org; Mon, 17 Jul 2017 13:39:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dX9yz-0003OF-0f for qemu-devel@nongnu.org; Mon, 17 Jul 2017 13:38:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50234) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dX9yy-0003MS-MY for qemu-devel@nongnu.org; Mon, 17 Jul 2017 13:38:56 -0400 Date: Mon, 17 Jul 2017 14:38:52 -0300 From: Eduardo Habkost Message-ID: <20170717173852.GT6020@localhost.localdomain> References: <20170711004303.3902-1-ehabkost@redhat.com> <20170711004303.3902-3-ehabkost@redhat.com> <6c43bf15-772e-2f8c-737c-6789d8ae28f2@linux.vnet.ibm.com> <20170712184805.GJ6020@localhost.localdomain> <93527f47-ad79-9c5b-d7ed-d33a089a024c@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <93527f47-ad79-9c5b-d7ed-d33a089a024c@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Halil Pasic Cc: qemu-devel@nongnu.org, Marcel Apfelbaum , Cornelia Huck , Greg Kurz On Sun, Jul 16, 2017 at 02:35:49PM +0200, Halil Pasic wrote: > On 07/12/2017 08:48 PM, Eduardo Habkost wrote: > > On Wed, Jul 12, 2017 at 08:06:14PM +0200, Halil Pasic wrote: > >> > >> > >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote: > >>> Test case to detect the bug fixed by commit > >>> "qdev: fix the order compat and global properties are applied". > >>> > >>> Signed-off-by: Eduardo Habkost > >> > >> Reviewed-by: Halil Pasic > >> > >> Please see below nevertheless. > >> > >>> --- > >>> tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++ > >>> 1 file changed, 33 insertions(+) > >>> > >>> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c > >>> index 48e5b73..ef2951f 100644 > >>> --- a/tests/test-qdev-global-props.c > >>> +++ b/tests/test-qdev-global-props.c > >>> @@ -33,6 +33,8 @@ > >>> #define STATIC_TYPE(obj) \ > >>> OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS) > >>> > >>> +#define TYPE_SUBCLASS "static_prop_subtype" > >>> + > >>> #define PROP_DEFAULT 100 > >>> > >>> typedef struct MyType { > >>> @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = { > >>> .class_init = static_prop_class_init, > >>> }; > >>> > >>> +static const TypeInfo subclass_type = { > >>> + .name = TYPE_SUBCLASS, > >>> + .parent = TYPE_STATIC_PROPS, > >>> +}; > >>> + > >>> /* Test simple static property setting to default value */ > >>> static void test_static_prop_subprocess(void) > >>> { > >>> @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void) > >>> g_test_trap_assert_stdout(""); > >>> } > >>> > >>> +/* Test if global props affecting subclasses are applied in the right order */ > >> > >> Now that I think about it this is affecting an external and > >> (end-)user facing interface. You say this is the right order. > >> Based on what is this the right order? Do we need to update some > >> documentation too? > > > > -global documentation is already very poor, as you have noticed: > > > >> > >> I've checked out the user manual. There we talk about 'driver' but > >> I could not find a spot where 'driver' is explained. > > > > Yes. For that reason, it looks like there's nothing to be > > updated on the existing (poor) documentation. I will re-read it > > to be sure. > > > > I don't agree. IMHO poor is reason enough to update. I agree we should update it. I just meant I don't see any existing documentation that will become incorrect/outdated when we apply this patch. > > >> > >> If I would have to translate between user manual terminology and > >> code terminology, I would say a driver is a not abstract class > >> inheriting from device. If that's right I wonder if it should > >> be allowed for a non-abstract class inheriting from device to > >> inherit form another non-abstract class. > > > > We could, but this wouldn't save us from having to decide what to > > do if people are already using those non-abstract superclasses > > with -global. e.g.: "-global spapr-pci-host-bridge.FOO=BAR" > > (spapr-pci-host-bridge is the parent of spapr-pci-vfio-host-bridge). > > > > This means a new rule like this would necessarily break > > command-line compatibility. Probably not a blocker in the > > spapr-pci-host-bridge case, but to me it looks like the rule > > would cause more problems than it would solve. > > We are already breaking the command line compatibility with this > series aren't we? > > You may be right about the cause more problems than it solves though. > I may end up thinking some more about this (or forgetting about it). Yeah. Breaking compatibility is not a blocker, but one additional obstacle for changing the rules. > > > >> > >> Another question is -global with an abstract class seems to be > >> accepted on the command line. Is that an undocumented feature? > > > > Considering that we never documented that "driver" could be a > > superclass, that's true: it is an undocumented feature. > > > > However, it is a feature people are likely to be relying upon, to > > configure a feature on all devices of a given type. > > > > Nod. I do however see a difference between abstract and non-abstract. I think I see your point. The semantics of "-global" would be clearer if all non-leaf classes were abstract. -- Eduardo