Re: [Qemu-devel] [RFC] QEMU Object Model status/merge plan

[Kevin Wolf]

Am 13.12.2011 17:02, schrieb Anthony Liguori:
> On 12/13/2011 09:05 AM, Kevin Wolf wrote:
>> Am 13.12.2011 14:43, schrieb Anthony Liguori:
>>> On 12/13/2011 05:35 AM, Stefan Hajnoczi wrote:
>>>> On Mon, Dec 12, 2011 at 7:36 PM, Anthony Liguori<address@hidden>   wrote:
>>>>> I choose the serial device to showcase what we'll eventually be able to 
>>>>> do.
>>>>>    The three relevant files are:
>>>>>
>>>>> https://github.com/aliguori/qemu/blob/qom-next/hw/isa-serial.c
>>>>>
>>>>> https://github.com/aliguori/qemu/blob/qom-next/hw/mm-serial.c
>>>>>
>>>>> https://github.com/aliguori/qemu/blob/qom-next/hw/serial.c
>>>>
>>>> I'm not sure I understand how init functions are called for derived
>>>> classes.
>>>
>>> There are three types of init functions:
>>>
>>> class_init
>>> ==========
>>>
>>> This lives in (TypeInit) and is called when a class is first created for a 
>>> type.
>>>    It is only ever called once.  Within this function, you should override 
>>> any
>>> methods in your base classes and set default implementations for any 
>>> methods you
>>> implement.
>>
>> I guess in most cases this could be replaced by a static table and the
>> function could be made optional? (That is, there could be a default
>> implementation for the NULL case)
> 
> As it turns out, you can pass an opaque to class_init so you could have 
> something like:
> 
> typedef struct PCIDeviceOps {
>     void (*foo)(PCIDevice *dev, ...);
>     ...
> };
> 
> And then you could write a generic:
> 
> void pci_generic_class_init(ObjectClass *klass, void *data)
> {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>      PCIDeviceOps *ops = data;
> 
>      if (ops->foo) {
>          k->foo = ops->foo;
>      }
> }
> 
> Which would then let you do:
> 
> static PCIDeviceOps e1000_device_ops = {
>      .foo = e1000_foo,
>      ...
> };
> 
> static TypeInfo e1000_device_info = {
>      .name = TYPE_E1000,
>      .parent = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(E1000State),
>      .class_init = pci_generic_class_init,
>      .class_data = &e1000_device_ops,
> };
> 
> I didn't really plan on this, but it looks like it would work pretty well.  
> It 
> might be reasonable to do for really common devices.  I don't think it's all 
> that nice to do for everything though because the *_generic_class_init() 
> functions get really ugly and makes allowing subclassing pretty hard.

It looks much nicer.

What kind of ugliness do you see in *_generic_class_init? Just that it's
a long chain of if (ops->field != NULL) k->field = ops->field;
statements? This looks like something that could easily be generated in
the long term.

Also, what's the problem with subclassing? Doesn't this work:

static TypeInfo not_quite_e1000_device_info = {
      .name = TYPE_NOT_QUITE_E1000,
      .parent = TYPE_PCI_DEVICE,
      .instance_size = sizeof(NotQuiteE1000State),
      .class_init = e1000_generic_class_init,
      .class_data = &not_quite_e1000_device_ops,
};

static PCIDeviceOps not_quite_e1000_device_ops = {
     .bar = not_quite_e1000_bar,
     .parent_ops = {
         .foo = not_quite_e1000_foo,
     },
};

void e1000_generic_class_init(ObjectClass *klass, void *data)
{
     E1000DeviceClass *k = E1000_DEVICE_CLASS(klass);
     E1000DeviceOps *ops = data;

     if (ops->bar) {
         k->bar = ops->bar;
     }

     pci_generic_class_init(klass, &ops->parent_ops);
}

(Btw, what's the point with klass vs. class? Do you expect that tools
which can deal with C++ will fail if we spell it class?)

>>> instance_init
>>> =============
>>>
>>> This is the constructor for a type.  It is called when an object is created 
>>> and
>>> chained such that the base class constructors are called first to 
>>> initialize the
>>> object.
>>
>> Same for this one, in your serial code it looks like this doesn't do
>> anything interesting in the common case and could be made optional (it
>> adds an UART child device, but this is static property and should be
>> moved anyway)
> 
> It could potentially, yes.  instance_init functions will often times not be 
> needed.  It's really meant to do things like initialize lists and stuff like 
> that that property accessors may need to work with.

Right, this is what I thought.

>> I think even in the future the really interesting work will be done in
>> realize.
> 
> Yes.  The various TypeInfo methods can all be omitted if they don't do any 
> work.

My concern is probably that currently they actually have to do some
work. Not much, just two or three lines of code, but it's enough to make
it impossible to omit them.

I think that QOM allows to do a lot of complicated things (at least you
have invested a lot of thought there, and even though I haven't seen the
patches yet, I'll assume that you got most of that right). The part that
I still see missing is that we should not only allow complicated things,
but also make the common thing simple.

Kevin