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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>