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

[Paolo Bonzini]

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: in the full proposal as I remember it,
link properties work on any QOM object; 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.  Make sure
that all required abstractions can be implemented in terms of a QOM
composition tree.

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

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?

(Regarding this, I'm partially guilty of the same with my SCSI
refactoring.  In the end almost everything came nicely out of the
refactoring, but I did have to backtrack a couple of times.  But the
damage I could do, and the churn caused by the backtracking, was much
more limited than with something like QOM!)

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;
+
+     /* Shouldn't this ref the child instead??  Also, where is the matching
+       unref??  Needs a release method? */
     qdev_ref(dev);

     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.

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

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

Going to bed now...

Paolo