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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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

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

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

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

> We jump too quickly at
> doing conversions without thinking through the underlying interface.

I hoped you had thought it through. ;)

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

> 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.  If you don't do 
this, you take the unmodifiable ABI and force its use as an API 
throughout all QOM.  We can do better.

>> 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.  Also, besides 
C primitive types there will be property types for structs: for example 
virtio or SCSI requests, S/G lists, and so on.

Paolo