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: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)
Date: Fri, 02 Dec 2011 20:40:06 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13

On 12/02/2011 06:56 PM, Paolo Bonzini wrote:
I have a few comments on the series.  I like it in general, but I
think it's way too incomplete to be a first incremental step, and it's
not clear what your plans are about this.  I think I could boil it
down to two requirements before committing this.

1) The biggest comment is that you have a huge difference between your
original proposal and this one:

It's actually exactly the same. It's just an incremental conversion of qdev into QOM. I can't think of a way to maintain compatibility (and sanity) without morphing qdev incrementally into QOM.

in the full proposal as I remember it,
link properties work on any QOM object;

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.

right now, however, buses are
not QOM-ified.

This is a problem because a qdev bus mediates between the parent and
children's interfaces.  You have the ingredients here for doing this
(the bus can be any device with a link to the parent and child
properties for the children; children also have backwards link), but
the bus in the middle right now is not QOM-ified and so the mediation
cannot be represented in the prototype.

Also, it is not clear how the bus will get hold of this interface from
the parent.  For the children it is a bit simpler, since the bus has
access to the DeviceInfo; I'm not sure however what the plan is and
whether you still plan on keeping the DeviceInfo descendents.

So, my #1 requirement for this to be committed is: Have all
relationships in the device tree expressed as properties.

This series is about one thing: introducing stable paths. The fact that link properties are being added is just because it's easy. I'm perfect happy pulling out link properties and waiting until we have enough of the object model to introduce links.

But I want to get started filling out the composition tree throughout QEMU. There's a lot of value in this. Most notably, we can switch to referencing devices based on their canonical path during live migration which means all of the non-sense around instance ids can disappear.

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. But you're getting ahead of yourself.


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 it's not clear how the infrastructure will evolve.  For example,
how do you plan to implement the realization phase that you had in the
original model?

That's step three. realize is a property but it's best implemented as a virtual function that is dispatched. We can't do this until we have virtual method dispatch.

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.

3) I still don't understand why the vtable need to be per-property and
not per-class.  I see no reason not to have DevicePropertyInfo
(statically defined) + DeviceProperty (a pointer to DevicePropertyInfo
+ the queue entry + name/type/whatever).  Please do make an attempt at
using composition to derive property types, as this is in general what
_you_ gave in the past as the direction for QEMU (see Notifiers).
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.

+
+     /* Shouldn't this ref the child instead??  Also, where is the matching
+       unref??  Needs a release method? */
      qdev_ref(dev);

Yup, that's a bug.  Thanks.

In terms of unref, we need to tie life cycles. That's probably something that should go in this patch (and this is how it works in my qom branch btw).


      g_free(type);
}

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.


Also in favor of containment and a property hierarchy, there is no
reason why Property cannot become a subclass of DeviceProperty.  The
DeviceInfo/BusInfo would contain a prototype that you can clone to the
heap when building the device's dynamic properties.  If instead you
want the declarative nature of qdev properties to be replaced by
imperative descriptions, that's fine, but please provide a proof of
concept of how to do the conversion (a perl script would be fine, even
if it works only for 2-3 device models).

I think you see just as well as I do that it's not that complicated. Working out the details of how properties are implemented are more important IMHO than starting to do conversions. We jump too quickly at doing conversions without thinking through the underlying interface.

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.

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.

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 wouldn't make this a requirement, but you really need a stronger
justification that what you gave so far.

4) I really hate naming something as "legacy", particularly when we
have hundreds of examples and no plan for converting them.  In fact,
there's nothing legacy in them except the string-based parsing.  Even
if you convert them away from the Property model to the visitor model,
they do not disappear, not at all.  They're simply not dynamic.  If
you call them legacy, I might be worried that I have to replace all

      dev->num_queues

with

     ((int) (intptr_t) dev->num_queues_prop->opaque)

So, my #2 requirement is to make it clear what the fate will be for
"legacy" properties, and possibly make enough conversion work that
they can just be called "static".  Do consider converting the
parse/print functions in Property to visitor-style (it's trivial).
Then provide a "legacy" bridge in the _reverse_ direction: there are
hundreds of qdev properties, and just a handful of users of them.

The trouble with the legacy properties are how they mix stringification with the properties themselves. In order to preserve compatibility, we're probably going to have to do something like mark all legacy properties deprecated, replace them with different named properties, and declare a flag day where we remove all legacy properties.

Yes, it sucks, but it's part of the ABI that we cannot now change. We have to treat the input to pcidevice.address as a hex integer in a string.

Regards,

Anthony Liguori

Going to bed now...

Paolo





reply via email to

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