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

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/03/2011 08:24 AM, Paolo Bonzini wrote:
> On 12/03/2011 03:40 AM, Anthony Liguori wrote:
>> That is still true. The next step, inheritance, will pull the properties
>> into a base class. That base class can be used elsewhere outside of the
>> device model.
>>
>> But this is already a 20 patch series. If you want all of that in one
>> series, it's going to be 100 patches that are not terribly easy to
>> review at once.
>
> Without a design document and a roadmap, however, it's impossible to try to
> understand how the pieces will be together. 100 patches may require some time to
> digest, but 20 patches require a crystal ball to figure out what's ahead.

You can see a bit further by looking at:

https://github.com/aliguori/qemu/commits/qom-next

That fills out the composition tree pretty well for the pc.  The next step is 
aggressive refactoring such that the qdev objects reflect the composition.  IOW, 
we should create the rtc from within the piix3 initialization function.

But before we can do that, we need to split construction and introduce realize 
which requires inheritance.

So I'd like to fill out the composition tree next (HEAD~2 in that branch, but 
split up nicely).  Having a flushed out composition tree really helps figure out 
what the code should look like and my hope is that other platforms can start 
refactoring too.

Inheritance is not terribly hard.  First step is adding a type pointer to 
DeviceState *, and then introducing some dynamic_cast-like macros and then 
touching every file to use them.

Second step is QOM-style methods and polymorphism.  That's not terribly hard either.

Once we have that, we can do realize.

Note that I haven't really talked about busses.  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.

>
>>> Make sure that all required abstractions can be implemented in
>>> terms of a QOM composition tree.
>>
>> Not composition tree, you mean via the link graph.
>
> I mean both, but the link graph is already understandable (1-to-1 is easy).
> 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.

>>> 2) Related to this, you have a lot of nice infrastructure, but (unlike
>>> what you did with QAPI) you haven't provided yet a clear plan for how
>>> to get rid of the old code. You also have only very limited uses of
>>> the infrastructure (see above).
>>
>> Old style properties go away as they're converted to new style properties.
>
> 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.

>> If you want to see how this all is going to look, look at the old tree
>> :-) That's where we're going to end up.
>
> Let's write documentation on that already.

I have written lots of documentation.  Just take a look at the wiki.  Nothing 
speaks better than code.

>>> It's better for various reasons--type safety and ease of use--even if
>>> it costs some boilerplate. For example the child property should be
>>> as simple as
>>>
>>> +struct ChildDeviceProperty {
>>> + DeviceProperty prop;
>>> + DeviceState *child;
>>> +}
>>> +
>>> +struct DevicePropertyInfo child_device_property_info = {
>>> + .size = sizeof(ChildDeviceProperty);
>>> + .get = qdev_get_child_property,
>>> +};
>>> +
>>> void qdev_property_add_child(DeviceState *dev, const char *name,
>>> DeviceState *child, Error **errp)
>>> {
>>> gchar *type;
>>>
>>> type = g_strdup_printf("child<%s>", child->info->name);
>>>
>>> - qdev_property_add(dev, name, type, qdev_get_child_property,
>>> - NULL, NULL, child, errp);
>>> + prop = (ChildDeviceProperty *)
>>> + qdev_property_add(&child_device_property_info,
>>> + dev, name, type, errp);
>>> +
>>> + /* TODO: check errp, if it is NULL -> return immediately. */
>>> + prop->child = child;
>>
>> This seems quite a bit uglier to me.
>
> I did say it costs some boilerplate, but it's once per type and you said there's
> a dozen of those. The type safety and more flexibility (an arbitrary struct for
> subclasses rather than an opaque pointer) is worth the ugliness.

I disagree.  Take a look at how string properties work in the above tree.  It's 
strongly type safe.  My old qom tree builds properties based on code generation 
too so the whole discussion on safety is somewhat moot.

>
>>> I think also that the type should not be part of DeviceProperty.
>>> Instead it should be defined by subclasses, with the
>>> DevicePropertyInfo providing a function to format the type to a
>>> string.
>>
>> I don't really know what you mean by this.
>
> 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.

>> We jump too quickly at
>> doing conversions without thinking through the underlying interface.
>
> I hoped you had thought it through. ;)

I have :-)  But I take your position as arguing that we should convert half the 
tree before merging anything which I disagree with.

>> 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.  As I said above, we really don't 
need to even worry about this until most of the work is done so let's avoid this 
debate for now.  The property infrastructure is flexible enough to support both 
models.

>> 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 :-)

I don't think it's usually what we want, but I'm not opposed to it.

>>> For artificial properties you would need one-off classes; but you can
>>> still provide a helper that creates a specialized
>>> DeviceProperty+DevicePropertyInfo from the functions. I'm thinking of
>>> how people implement prototype-based OO on top of class-based OO, but
>>> it might be just a macro.
>>
>> I think you're over thinking the problem. There are going to be maybe a
>> dozen property types and that's it. They all with correspond exactly to
>> C types with the exception of links/children.
>
> 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.

Regards,

Anthony Liguori