Re: [Qemu-devel] [PATCH 27/28] sysbus: apic: ioapic: convert to QEMU Object Model

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/25/2012 11:27 AM, Jan Kiszka wrote:
> >  I agree with Anthony, this would get really ugly where you are calling
> >  the functions and doing the class initialization.
>
> I think we need to try it first.
>
> There is a lot of repetition, and that gets boring at beat and ugly at
> worst when doing it for hundreds of devices - compared to the number of
> base classes we will have.

I think we have to trust Anthony's judgement that he tried it and didn't 
like it.  Perhaps he didn't try _exactly_ the version that you gave, but 
I think it's not fair to say "please try redoing it this way---maybe" a 
month after the first version has been posted.

So far, we were always able to reach a compromise between Anthony's 
taste and others', and also on the order with which to do things.  I 
understand this is a pretty major issue, but it's going to be one more 
such compromise (and one that I thought had already been settled).

Note that it's not impossible to change directions!  QOM started with 
the idea of converting devices _last_.  We are converting them _first_. 
  That was a very early change in plans, and very much against Anthony. 
  It's fair and natural that the balance swings in the other direction 
now.  As a reviewer, the later you join the party, the harder it will be 
to swallow the pill.

> >  It's a different
> >  style from what we're used to, granted, but the difference in code size
> >  is not relevant (not enough to introduce a level of macro magic, at
> >  least) and the diffstat in this series is misleading because qdev is
> >  left with temporary duplication for now.
>
> I was looking at the final version Anthony pointed at.

There _are_ things that leave something to be desired in there.  For 
example I don't like accessing the base class with a different name, as in:

     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

     dc->props = e100_properties;
     dc->desc = info->desc;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     k->romfile = "pxe-eepro100.rom";
     ...

and a few more things that we'll discuss as soon as he posts the third 
instalment.  I know we're all busy, but the only way to solve these 
problems is prompt review.

Paolo