On Thu, Jul 05, 2012 at 05:00:00PM +0200, Andreas Färber wrote: > (Dropping some borked CCs) > > Am 05.07.2012 16:15, schrieb Michael S. Tsirkin: > > On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote: > >> 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<afaer...@suse.de> > >>>>>>> --- > >>>>>>> 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. > > > > It's important but not the most important thing. > > It does not make sense to do casts *everywhere*. > > Do it where it makes sense. > > > >> 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 > > > > So far QOM was a win. > > You examples look better with a macro. Patch 13 looks better: > > - PCIHostState *phb = FROM_SYSBUS(PCIHostState, > > SYS_BUS_DEVICE(s->pcihost)); > > + PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost); > > this is an improvement: devices do not want to deal with sysbus. > > > > The code above that is being changed looks better without. > > Do you want me to apologize for my macro misuse now? I apologize. :) > Replace PCI_HOST_BRIDGE(s)->bus = b; with > PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s); > phb->bus = b; > in your mind and it matches exactly what you liked better above, no?
No, what I liked above is that we hide sysbus from devices. > > This is why this thread is about the specific patch and not > > general QOM. > > You're basically saying, QOM was a win but don't use it here. That's a > contradiction and thus a general QOM issue since that paradigm not only > applies here: Either we need to design all structs so that their fields > have nice names to access directly as you suggest, or nobody must access > the parent field as Anthony suggests. Having a wild mix of both at > maintainers' gusto is not a good solution. With time we'll end up with a mix anyway since compiler does not enforce no direct access. > Arguably we can just leave out this last patch if it is so > controversial. However, the split is arbitrary since I backwards-coded > this series, moving code snippets to earlier individual patches using > checkout -p, i.e. patches 1-11 have this final code design in mind and > thus went from SYS_BUS_DEVICE() to PCIHostState above rather than > through parent_obj. The reason I couldn't do it there directly is that > we didn't have the PCI_HOST_BRIDGE() macro before patch 13. > I also based patch 14 on the assumption that i440fx will get more state > fields when Anthony goes ahead with his QOM Pin series, otherwise we > could just scratch the I440FXState typedef (same discussion as for the > qtest herbivore test case). > > Andreas The real problem is that devices have to tweak pcihost->bus and that is because we don't model has-a relationship between pci host bridge and bus well: pci host bridge has a pci bus. As long as that remains true it seems to me the more explicit it is the better, so it's easier to fix. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >