qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and compositio


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)
Date: Sat, 03 Dec 2011 15:24:04 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]