From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dWiYE-0005Fr-9N for qemu-devel@nongnu.org; Sun, 16 Jul 2017 08:21:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dWiYA-0002Rc-Vt for qemu-devel@nongnu.org; Sun, 16 Jul 2017 08:21:30 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:40730) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dWiYA-0002P5-Ld for qemu-devel@nongnu.org; Sun, 16 Jul 2017 08:21:26 -0400 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v6GCItgE099827 for ; Sun, 16 Jul 2017 08:21:22 -0400 Received: from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109]) by mx0a-001b2d01.pphosted.com with ESMTP id 2bqfkffvgs-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Sun, 16 Jul 2017 08:21:22 -0400 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 16 Jul 2017 13:21:20 +0100 References: <20170711004303.3902-1-ehabkost@redhat.com> <20170711004303.3902-2-ehabkost@redhat.com> <91940c7e-3685-26e0-e8f9-e050afcd28b5@linux.vnet.ibm.com> <20170712182929.GI6020@localhost.localdomain> <640df239-d674-747d-d6d1-692030908c43@linux.vnet.ibm.com> <20170713161557.GU6020@localhost.localdomain> From: Halil Pasic Date: Sun, 16 Jul 2017 14:21:16 +0200 MIME-Version: 1.0 In-Reply-To: <20170713161557.GU6020@localhost.localdomain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Message-Id: Subject: Re: [Qemu-devel] [PATCH 1/3] qdev: fix the order compat and global properties are applied List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Marcel Apfelbaum , Cornelia Huck , Greg Kurz On 07/13/2017 06:15 PM, Eduardo Habkost wrote: > On Thu, Jul 13, 2017 at 01:54:01PM +0200, Halil Pasic wrote: >> >> >> On 07/12/2017 08:29 PM, Eduardo Habkost wrote: >>> On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote: >>>> >>>> >>>> On 07/11/2017 02:43 AM, Eduardo Habkost wrote: >>>>> From: Greg Kurz [..] >>>> Code looks good to me. Given that high profile people are happy with the >>>> patch I won't object on the behavior aspect which I don't understand fully. >>>> Thus: >>>> >>>> Reviewed-by: Halil Pasic >>>> >>>> >>>> Now a couple of questions for keeping my clear conscience: >>>> * What did we test? Since this patch is fixing a problem it >>>> changes behavior. Did we test if there is something that breaks? >>>> >>>> * The previous version seems to establish a (somewhat strange) >>>> precedence for the case the same device property (storage object) >>>> is set via multiple super-classes (e.g. set both by parent and >>>> parents parent). This seems to have at least been possible, >>>> and technically it still is I guess. Now instead of most general >>>> (super class) wins we have last added property wins. I assume it >>>> isn't a problem, because we don't have something obscure like that. >>>> Or am I wrong? This obviously connects to the first question. >>>> (By the way, most specialized wins would not have been that >>>> surprising given how inheritance and OO usually works. My assumption >>>> that nobody used this obscure mechanism is largely based on it's >>>> strangeness). >>> >>> Note that we are not changing the behavior when the classes >>> themselves set different defaults. Subclasses are still free to >>> override defaults set by superclasses inside QEMU code, and they >>> will be unaffected by this series. What we are changing here are >>> the semantics of the -global command-line option when applied to >>> superclasses. >> >> I was not referring to this. > > Good. :) > >> >>> >>> The main sources of global properties we have are: >>> * MachineClass::compat_props> * -global command-line option >> >> I was thinking about these two. > > Good, this is what we're really trying to address, so let's > review that: > >> >>> * -cpu command-line option >>> >>> The behavior on the compat_props case was addressed by the hack >>> in commit 0bcba41f "machine: Convert abstract typename on >>> compat_props to subclass names". This means compat_props was >>> already following the order in which properties were registered. >> >> I disagree. Commit 0bcba41f affects only *abstract* classes. So >> your statement is true if a non-abstract class inheriting form >> device can only have abstract ancestor classes. I'm not aware >> such rule exists in QEMU, and according to your reply to my comment >> on patch 2 there are even people using non-abstract superclasses >> for devices. > > Good catch! You are correct, and your experiment below is > correct. But I thought no non-abstract superclasses where used > on compat_props on any machine-type (then this patch wouldn't > have any visible effect in the current tree). > > However, commit 0bcba41f has this note, which I had forgot about: > > Note that there's one case that won't be fixed by this hack: > "-global spapr-pci-vfio-host-bridge.