Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/05/2011 03:52 AM, Paolo Bonzini wrote:
> On 12/04/2011 10:01 PM, Anthony Liguori wrote:
> > >
> >
> > I've begun the work of introducing proper inheritance. There's a
> > lot going on but the basic idea is:
> >
> > 1) introduce QOM base type (Object), make qdev inherit from it
> >
> > 2) create a dynamic typeinfo based DeviceInfo, make device class
> > point to deviceinfo
> >
> > 3) model qdev hierarchy in QOM
> >
> > 4) starting from the bottom of the hierarchy, remove DeviceInfo
> > subclass and push that functionality into QOM classes
>
> Ok, now we're talking. I looked at the SCSI bits and it looks
> sane---though I still don't understand your loathe for static
> initializers (which also causes a lot of code churn).

It's to support method inheritance.  In qdev, the various DeviceInfo structures 
correspond roughly to the class of the object.  When you create an ISADeviceInfo 
(the ISA subclass), you declare it statically.

Any methods you aren't overriding are going to be initialized to zero.  You want 
those methods to inherit their values from the base class.  To do this in qdev, 
you have to introduce a base-class specific registration function 
(isa_qdev_register).

There's not a lot of discipline in how these functions are implemented and 
generally makes type registration more complicated as you have to understand 
what methods get overridden.

What QOM does (GObject does this too) is use a function to override methods.  I 
agree that it's not as pretty, but it means you can just memcpy() the parent 
class' contents into the derived class.  This makes inheritance much cleaner and 
also means you can register all types through a single interface.

You can see this refactoring happening in that branch.  I'm slowly getting rid 
of all of the DeviceInfo derivatives.  Once everything is using DeviceInfo 
directly, then I'll replace DeviceInfo with TypeInfo.  This is going to be the 
biggest single change in the QOM conversion.  It's going to have to be done by a 
script.

Getting rid of SysBusInfo will probably also require a script.

> > I do want to get rid of qdev busses but I'm in no rush at all. We
> > can do 95% of the necessary refactoring without touching busses and
> > hot plug and I think that's the right strategy.
>
> Yes, as long as you have a plan for them. Some of the buses have data
> so they will have to become objects of their own, with a bidirectional
> link between them and the parent device. (Because I'm not going to
> write code like this for each HBA:
>
> void lsi_set_unit_attention(Device *dev, SCSISense sense)
> {
> LSIDevice *lsi = LSI_DEVICE(dev);
> lsi->unit_attention = sense;
> }
>
> No, I'm not. Over my dead body).

Yes, my current plan is to make buses interfaces which means you would need to 
have getters/setters for state.  Yes, I know that can get ugly quickly.

But this comes down to refactoring the bus interfaces I think.  I honestly can't 
tell you how I would handle this cleaner without sitting down and thinking about 
the SCSI bus interface.

Generally speaking, accessors are a very good thing.  It makes code much easier 
to refactor and you'll notice that a lot of my refactoring patches start by 
adding accessors/wrapper functions that probably should have been there since 
day one.

But I agree, we don't want to end up with something where you add 10 lines 
instead of one line.

> > > Right now I can hardly understand how the composition tree will
> > > work, for example: how do you constrain children to be subclasses
> > > of some class (or to implement some interface)?
> >
> > Composition never involves subclasses. The tree I point you to above
> > is about as complete as it can be right now without doing more qdev
> > conversions. It should answer all of your questions re: composition.
>
> All except one: why can't child (and link too IIRC) properties be created with a
> NULL value?

links are nullable and usually start out as NULL.

childs are not nullable. I can't really think of a reason why they should be 
nullable.  What are you thinking here?

> > > And how do they? How much code churn does that entail, in the
> > > devices and in general? In fact, why do they need conversion at
> > > all? Static properties are special enough to warrant something
> > > special.
> >
> > They can stay around forever (or until someone needs non-static
> > properties) but I strongly suspect that we'll get rid of most of them
> > as part of refactoring.
> >
> > An awful lot of properties deal with connecting to back ends and/or
> > bus addressing. None of that will be needed anymore.
>
> True for PCI, but what about ISA or MMIO devices? They do their own
> address/IRQ decoding. You don't see those properties in many cases
> because they're hidden beneath sysbus magic, but there are hundreds.

I've thought a lot about bus properties.  I've looked at a lot of code at this 
point and for the most part, I think that the reason they even exist is because 
we can't inherit a default set of properties.

SCSI is a good example.  The bus properties really make more sense as SCSIDevice 
properties that are inherited.

I dislike these properties in the first place, but I'd like to find a way to 
convert everything to the QOM type system before we start rearchitecting how 
hotplug works.

> > > Let's write documentation on that already.
> >
> > I have written lots of documentation. Just take a look at the wiki.
>
> It's down. :(

It's up now.

> > > Once the type is written as child<RTCState>, you've lost all info
> > > on it. Each property type should have its own representation for
> > > types (it can be implicit, or it can be an enum, or it can be a
> > > DeviceInfo), with a method to pretty-print it.
> >
> > I don't know what you mean by "lost all info on it" but I'm strongly
> > opposed to a "pretty-print" method. Having the pretty printing
> > information in the type is almost never what you want.
>
> I agree---I was talking about pretty-printing *the type itself*. The
> type falls outside the visitor model, which is only concerned about the
> contents.
>
> > > > You may find it odd to hear me say this, but I grow weary of
> > > > adding too much class hierarchy and inheritance with properties.
> > > > Yes, we could make it very sophisticated but in practice it
> > > > doesn't have to be. Properties should be one of two things:
> > > > primitive types or link/childs.
> > >
> > > ... and interfaces. Accessing them by name as a property should
> > > work well.
> >
> > No, not interfaces. The object *is-a* interface. It doesn't has-a
> > interface.>
> >
> > We've gone through this debate multiple times.
>
> You misunderstood (and I wasn't clear enough).
>
> The is-a/has-a debate is indeed settled for things such as PCI-ness
> where we'll keep the current three-level class hierarchy (two abstract,
> one concrete):
>
> Device
> PCIDevice
> ... concrete PCI devices ...
> ISADevice
> ... concrete ISA devices ...
>
> The problem is that, in addition to the is-a class hierarchy, you have
> the interface hierarchy. Here, C-level struct "inheritance" does not
> help you because you do not have a fixed place for the vtable. So, for
> example, instead of
>
> struct SCSIBusInfo {
> .command_complete = lsi_command_complete,
> ...
> };
>
> I'll have something like this:
>
> Interface *interface;
> SCSIBusIface *sbi;
> interface = qdev_add_interface(&dev->qdev, TYPE_SCSI_BUS_INTERFACE);
> sbi = SCSI_BUS_INTERFACE(interface);
> sbi->command_complete = lsi_command_complete;
> ...
>
> Right? Then I create the SCSIBus object with something as simple as:
>
> struct LSIDevice {
> ...
> SCSIBus *bus;
> }
>
> dev->bus = scsi_bus_new(&dev->qdev);
>
> and scsi_bus_new will do something like
>
> Interface *interface;
> interface = qdev_get_interface(dev, TYPE_SCSI_BUS_INTERFACE);
> scsi_bus->sbi = SCSI_BUS_INTERFACE(interface);


No, you would do:

struct SCSIBus {
   Interface parent;
   void (*command_complete)(SCSIBus *bus, SCSIRequest *req);
};

TypeInfo scsi_bus_info = {
   .name = TYPE_SCSI_BUS,
   .parent = TYPE_INTERFACE,
};

type_register_static(&scsi_bus_info);

--------

struct LSIDevice {
    PCIDevice parent;
};

static void lsi_command_complete(SCSIBus *bus, SCSIRequest *req)
{
   LSIDevice *dev = LSI_DEVICE(bus);
   ...
}

static void lsi_scsi_bus_initfn(Interface *iface)
{
   SCSIBus *bus = SCSI_BUS(iface);

   bus->command_complete = lsi_command_complete;
}

TypeInfo lsi_device_info = {
  .name = TYPE_LSI,
  .parent = TYPE_PCI_DEVICE,
  .interfaces = (Interface[]){
     {
        .name = TYPE_SCSI_BUS,
        .interface_initfn = lsi_scsi_bus_initfn,
     }, {
     }
  },
};

type_register_static(&lsi_device_info);

> Perhaps hidden with some macro that lets me just write
> SCSI_BUS_INTERFACE(dev), but that's the idea; such a lookup function is
> pretty much what all object models do. GObject has
> G_TYPE_INSTANCE_GET_INTERFACE, COM/XPCOM has QueryInterface, etc.
>
> If I understood everything so far, then here is my question. Are
> interfaces properties?

No.  A device is-a interface.  Hopefully the above example will make it more 
clear.

> I'm starting to understand now that the answer
> is probably "no because interfaces have nothing to do with visitors",
> and that's fine. Then I suppose you'll have another list to lookup
> besides properties, no problem with that.

Yes, there's a list of interfaces embedded in the Object type.

> > > > This is where qdev goes very wrong. It mixes up user interface
> > > > details (how to format an integer on the command line) with
> > > > implementation details. There's no reason we should be indicating
> > > > whether a property should be display in hex or decimal in the
> > > > device itself.
> > >
> > > That's totally true. But there's no reason why qdev properties
> > > cannot be split in two parts, a sane one and a legacy one.
> >
> > Bingo! Hence, the 'legacy<>' namespace. If you want to do a
> > declare, struct member based syntax that encodes/decodes as primitive
> > types to a Visitor, be my guest
>
> That's not what I meant. The legacy<> namespace splits the set of QOM
> properties in two parts, sane ones and legacy ones. That's wrong,
> because the old broken interface remains there. Worse, it remains
> there as user-visible API in the JSON etc., and it will remain forever since we
> cannot get rid of -device overnight.
>
> What I suggested is to provide two implementations for each old-style
> property: an old string-based one (used for -device etc.) and a modern
> visitor-based one (used for qom_*). In other words, old-style
> properties would expose both a print/parse legacy interface, and a sane
> get/set visitor interface. No need for a legacy<> namespace, because
> new-style consumers would not see the old-style string ABI.

Yeah, I'd like to do something like this but I'm in no rush.  I agree that when 
we declare QOM as a supported interface, we should have replacements for 
anything that's in the legacy<> space.  That may be from some magic Property 
tricks where we introduce Visitor to parse/print or because we introduce new and 
improved properties.

Maybe now is the right time to rename the legacy properties to all be prefixed 
with qdev-?  That way we don't need to introduce two different types for a 
single property.

Regards,

Anthony Liguori

> > > I was thinking of one-off types such as rtc's struct tm.
> >
> > I'd really like to get to a point where these type visitors were
> > being generated.
>
> Yeah, long term that'd be great.
>
> Paolo