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

[Anthony Liguori]

On 12/14/2011 04:33 AM, Kevin Wolf wrote:
> Am 13.12.2011 17:02, schrieb Anthony Liguori:
> > 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?

Yeah.  The other thing that gets a bit weird about it is a common trick in order 
to still be able to call the superclass version of the function is to squirrel 
away the super class pointer in a class member.  Something like:

static void e1000_handle_event(PCIDevice *dev)
{
    E1000State *s = E1000_DEVICE(dev);
    E1000Class *k = E1000_DEVICE_GET_CLASS(s);

    // Call base class version of the function
    k->super_handle_event(dev);

    // Implement subclass behavior for function
}

static void e1000_class_init(ObjectClass *klass, void *data)
{
    PCIDeviceClass *pk = PCI_DEVICE_CLASS(klass);
    E1000Class *ek = E1000_CLASS(klass);

    ek->super_handle_event = pk->handle_event;
    pk->handle_event = e1000_handle_event;
}

There is an object_get_super() that returns the superclass that would let you do 
the same thing but the above is the idiomatic form in GObject at least.

> 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);
> }

It definitely works, it just ends up adding boiler plate.  One of my goals was 
to make it as easy as possible to specify a device with a little code as possible.

I don't think there's much of a readability gain by using an ops structure. 
It's basically:

 .init = e1000_device_init,

vs:

  k->init = e1000_device_init;

The only advantage I see in using an ops structure is that it prevents you from 
doing anything other than overriding members. I'm not sure that that is a virtue.

> (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?)

Since I was doing so much code conversion, I tried to err on the side of being 
conservative so that we wouldn't have to touch everything again.  So I avoided 
any C++ keywords as much as possible.

Perhaps one day we'll export a libqemu-block.so and even though it will be C, 
we'll want to make sure that a C++ program can include it.

> > > > 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.

Yeah, I think it's pretty simple right now.  If you want to add a method to a 
class, you:

1) Add it to the <DEVICE>Class structure (adding the structure if necessary)
2) Set the default value of that method in the class_init function for that type

If you want to override a base class method, you:

1) Set a new value for the appropriate method in your type's class_init function

Contrast this with qdev, where to add a method, you:

1) Need to make sure a <BUS>DeviceInfo structure exists

2) Potentially update the bus registration function if a default version needs
to be implemented

There's no assumption of infrastructure in QOM.  All types are self contained. 
You actually can't override a base class method in qdev unless it's explicitly 
supported by the bus registration function (and most of them don't allow it).

Maybe we can find ways to make common simple things simpler down the road, if 
so, I'm all for it.

Regards,

Anthony Liguori

> Kevin