From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34561) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmmVo-0002v8-07 for qemu-devel@nongnu.org; Thu, 05 Jul 2012 09:54:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmmVl-0007Cn-K4 for qemu-devel@nongnu.org; Thu, 05 Jul 2012 09:54:27 -0400 Received: from mail-qc0-f173.google.com ([209.85.216.173]:35432) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmmVl-0007Bw-G2 for qemu-devel@nongnu.org; Thu, 05 Jul 2012 09:54:25 -0400 Received: by qcab12 with SMTP id b12so3948705qca.4 for ; Thu, 05 Jul 2012 06:54:23 -0700 (PDT) Message-ID: <4FF59C8D.8040706@codemonkey.ws> Date: Thu, 05 Jul 2012 08:54:21 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1341422373-13614-1-git-send-email-afaerber@suse.de> <1341422373-13614-15-git-send-email-afaerber@suse.de> <20120704211717.GC27653@redhat.com> <20120704212614.GA27711@redhat.com> <4FF4C4EC.2090404@suse.de> <20120705133431.GF30572@redhat.com> In-Reply-To: <20120705133431.GF30572@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: jbaron@redhat.com, Alexander Graf , qemu-devel@nongnu.org, =?ISO-8859-1?Q?Andreas_F=E4rber?= , "\"list@suse.de@zmta06.collab.prod.int.phx2.redhat.com\": New World" , pbonzini@redhat.com, open@suse.de, =?ISO-8859-1?Q?Andreas_F=E4rber?= On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote: > On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote: >> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin: >>> On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrote: >>>> On Wed, Jul 04, 2012 at 07:19:33PM +0200, Andreas Färber wrote: >>>>> Uglify the parent field to enforce QOM-style access via casts. >>>>> Don't just typedef PCIHostState, either use it directly or embed it. >>>>> >>>>> Signed-off-by: Andreas Färber >>>>> --- >>>>> hw/alpha_typhoon.c | 4 ++-- >>>>> hw/dec_pci.c | 2 +- >>>>> hw/grackle_pci.c | 2 +- >>>>> hw/gt64xxx.c | 26 ++++++++++++++++---------- >>>>> hw/piix_pci.c | 6 ++++-- >>>>> hw/ppc4xx_pci.c | 8 +++++--- >>>>> hw/ppce500_pci.c | 2 +- >>>>> hw/prep_pci.c | 8 +++++--- >>>>> hw/spapr_pci.c | 12 +++++++----- >>>>> hw/spapr_pci.h | 2 +- >>>>> hw/unin_pci.c | 14 +++++++------- >>>>> 11 files changed, 50 insertions(+), 36 deletions(-) >>>>> >>>>> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c >>>>> index 58025a3..955d628 100644 >>>>> --- a/hw/alpha_typhoon.c >>>>> +++ b/hw/alpha_typhoon.c >>>>> @@ -46,7 +46,7 @@ typedef struct TyphoonPchip { >>>>> OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BRIDGE) >>>>> >>>>> typedef struct TyphoonState { >>>>> - PCIHostState host; >>>>> + PCIHostState parent_obj; >>>>> >>>>> TyphoonCchip cchip; >>>>> TyphoonPchip pchip; >>>>> @@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus, >>>>> b = pci_register_bus(dev, "pci", >>>>> typhoon_set_irq, sys_map_irq, s, >>>>> &s->pchip.reg_mem, addr_space_io, 0, 64); >>>>> - s->host.bus = b; >>>>> + PCI_HOST_BRIDGE(s)->bus = b; >>>>> >>>>> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */ >>>>> memory_region_init_io(&s->pchip.reg_iack,&alpha_pci_iack_ops, b, >>>> >>>> Sorry I don't understand. >>>> why are we making code ugly apparently intentionally? >>> >>> Just to clarify: replacing upcasts which are always safe >>> with downcasts which can fail is what I consider especially ugly. >> >> As per Anthony the parent field in the QOM instance structs is not >> supposed to be touched (cf. object.h). We mark it /*< private>*/ so >> that it doesn't even show up in gtk-doc documentation. > > PCIHostState is not private here. And if it were, it won't be > of any use because compiler does not read gtk-doc documentation > and neither do developers. > >> If it is unused, >> its name becomes irrelevant and could even be "reserved" if we so >> wanted. Renaming it to whatever proves that all old references are gone. > > Adding arbitrary rules like that only seems to make code future proof. > People will not remember and will use the field anyway, > then when you try to change it there will be work to be done. > So let's do the work when we really need it. > >> Background is that qdev and QOM work differently with regards to >> inheritance: as mentioned in the preceding patch, for qdev the parent >> was (had to be) identified by name and could be anywhere in the struct; >> for QOM the parent is a subset of the struct from the start and it's >> supposed to be accessed through the struct type that provides the >> fields, the usual way to get such a pointer is through >> OBJECT_CHECK()-derived cast macros. > > It makes sense if you go from parent to child. Even in C++ > you don't use dynamic_cast to go from child to parent. But you use static cast. Casting is not the same thing as dereferencing a member. IOW: Foo *foo = (Foo *)bar; Is very different than: Foo *foo = &bar.foo; Using cast macros makes the code an awful lot more readable because it tells the reader more. Foo *foo = FOO(bar); Tells the user that we're casting bar to the a type Foo. OTOH: Foo *foo = &bar.foo; Doesn't tell the user anything. A FOO() macro is consistent regardless of how the type is implemented too. It gets even worse when you are going up multiple levels: DeviceState *dev = &bar.host.qdev; Is unintelligible whereas: DeviceState *dev = DEVICE(bar); Tells you exactly what's happening. But really, this isn't the place to debate this. QOM has been around for a while. Consistency is important and this is how things are done in QOM. If you want to revisit this style, you should start a separate thread about it and we can talk about it. But this patch is consistent with the current infrastructure. Regards, Anthony Liguori > > >> In the snippet above I chose to use the macro directly since there was >> no second use but I can introduce a variable if preferred. >> What I see now though is that this code snippet creating the PCIBus >> should be moved up into the SysBus init function. >> >> I'll wait with posting a v4 til I hear what exactly to rebase it on. >> As for further renaming/splitting wishes this is what I referenced: >> http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html >> >> In particular, did you expect me to rename PCIHostState to PCIHostBridge >> / PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did >> you just mean the new PCI_HOST_BRIDGE macros like I did in this v3? >> >> Regards, >> Andreas > > Yes just the new macros. > >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >>