Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 06:35:06PM +0100, Kevin Wolf wrote:
> Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben:
> > > > Looks nice as end goal.  Then, these are a few questions I would
> > > > have about the transition plan:
> > > > 
> > > > Would it require changing both device implementation and device
> > > > users in lockstep?  Should we have a compatibility layer to allow
> > > > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > > > the devices are converted to the new system?  If not, why not?
> > > 
> > > Technically, it doesn't strictly require a lockstep update. You can have
> > > two code paths leading to a fully initialised device, one being the
> > > traditional way of setting properties and finally calling dc->realize,
> > > the other being a new method that takes the configuration in its
> > > parameters and also sets dev->realized = true at the end.
> > > 
> > > If at all possible, I would however prefer a lockstep update because
> > > having two paths is a weird intermediate state and the code paths could
> > > easily diverge. Keeping the old way around for a device also means that
> > > property setters are still doing two different jobs (initial
> > > configuration and updates at runtime).
> > 
> > I'd like to understand better how that intermediate state would
> > look like and why there's risk of separate code paths diverging.
> >
> > Could we have an intermediate state that doesn't require any
> > duplication and thus have no separate code paths that could
> > diverge?
> 
> The one requirement we have for an intermediate state is that it
> supports both interfaces: The well-know create/set properties/realize
> dance, and a new DeviceClass method, say .create(), that takes the
> configuration in parameters instead of relying on previously set
> properties.

I agree completely.

> 
> I assumed two separate implementations of transferring the configuration
> into the internal state. On second thought, this assumption is maybe
> wrong.
> 
> You can implement the new method as wrapper around the old way: It could
> just set all the properties and call realize. Of course, you don't win
> much in terms of improving the class implementation this way, but just
> support the new interface, but I guess it can be a reasonable
> intermediate step to resolve complicated dependencies etc.
> 
> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.

Comment about this below (look for [1]).

> 
> As you have worked a lot with properties recently, maybe you have a good
> idea how we could get an intermediate state closer to this?

I'd have to re-read this whole thread and think about it.

> 
> > > > If we add a compatibility layer, is the end goal to convert all
> > > > existing qdev_new() users to the new system?  If yes, why?  If
> > > > not, why not?
> > > 
> > > My personal goal is covering -object and -device, i.e. the external
> > > interfaces. Converting purely internally created devices is not as
> > > interesting (especially as long as we focus only on object creation),
> > > but might be desirable for consistency.
> > 
> > I wonder how much consistency we will lose and how much confusion
> > we'll cause if we end up with two completely separate methods for
> > creating devices.
> 
> I do think we should follow through and convert everything. It's just
> not my main motivation, and if the people who work more with qdev think
> it's better to leave that part unchanged (or that it won't make much of
> a difference), I won't insist.

This worries me.  Converting thousands of lines of code that
don't involve user-visible interfaces seems complicated and maybe
pointless.  On the other hand, having two separate APIs for
creating objects internally would cause confusion.

Maybe we should accept the fact that the 2 APIs will exist, and
address the confusion part: we should guarantee the two APIs to
be 100% equivalent, except for the fact that the newer one gives
us type safety in the C code.

I'd like to avoid a case like qdev vs QOM APIs, where they have
similar but slightly different features, and nobody knows which
one to use.

> 
> > > > What about subclasses?  Would base classes need to be converted
> > > > in lockstep with all subclasses?  How would the transition
> > > > process of (e.g.) PCI devices look like?
> > > 
> > > I don't think so.
> > > 
> > > If you want to convert base classes first, you may need to take the
> > > path shown above where both initialisation paths coexist while the
> > > children are converted because instantiation of a child class involves
> > > setting properties of the base class. So you can only restrict these
> > > properties to runtime-only after all children have been converted.
> > > 
> > > The other way around might be easier: You will need to describe the
> > > properties of base classes in the QAPI schema from the beginning, but
> > > that doesn't mean that their initialisation code has to change just yet.
> > > The child classes will need to forward the part of their configuration
> > > that belongs to the base class. The downside is that this code will have
> > > to be changed again when the base class is finally converted.
> > > 
> > > So we have options there, and we can decide case by case which one is
> > > most appropriate for the specific class to be converted (depending on
> > > how many child classes exist, how many properties are inherited, etc.)
> > 
> > Right now it's hard for me to understand what those intermediate
> > states would look like.  It sounds like it requires too many
> > complicated manual changes to be done by humans, and lots of room
> > for mistakes when maintaining two parallel code paths.  I'd
> > prefer to delegate the translation job to a machine.
> 
> Maybe devices are in a better shape, but my conclusion from user
> creatable objects is that it needs to be done by a human.

I agree with the "done by a human part", but I prefer a different
approach: making the APIs converge (using human work), and then
making an automated transition.

More on this below:

> 
> Even just writing a schema for an existing object without actually
> changing its code (i.e. what this series does for user creatable
> objects) involves:
> 
> * Figuring out which properties even exist.
> 
>   I guess if you know your way around QOM, this could be automatically
>   be generated for the common case. If property definitions are
>   conditional or dynamic, you'll probably miss them.

With qdev static properties and class-level properties, this
stage is easy.

> 
> * Finding the right data type for each property.
> 
>   The const char *type passed to object_(class_)property_add() is often
>   enough wrong that it becomes useless. If object_property_add_<type> is
>   used, chances are that you know the right type, but strings might
>   actually be hidden enums. If not, figuring out the type involves
>   analysing the setter and getter callbacks.

With object_property_add_*() and object_class_property_add_*(),
this part is very difficult.

With qdev static properties (now known as "field properties" in
QOM), this stage is easy.

Luckily, most devices use qdev static properties.


> 
> * Finding out which properties are mandatory and which are optional.
> 
>   This is usually some handwritten check in complete/realize that
>   returns an error. Sometimes it's just a segfault that happens sooner
>   or later if the property hasn't been set.

This stage is tricky.  But we can start by:
- Making all of them optional by default (which is true);
- Adding a flag to allow us to declare properties as mandatory.

> 
> * Finding the default for documentation.
> 
>   There are multiple ways to do this. It's code, not data.

In this case, qdev static properties / field properties make that
job possible.

> 
> * Writing (preferably good) documentation for the property.

This part will require manual work.

> 
> I see very little opportunity for automating a significant part of this.

I see a big opportunity to automate this if the device is already
using the field property system (which is true for most of the
devices).

The remaining work that needs to be done by humans is converting
existing dynamic properties to field properties.


About doing the compatibility layer the other way around[1]: if
conversion from qdev properties to QAPI schema is automated, we
can simply choose a point in time where the results of the
conversion would be saved in the git tree, and the original
property list would be deleted from the C source.  Then, the
compatibility layer would work the other way around.


> 
> Once you have this information, going to the intermediate state where
> .create() is just a wrapper that sets properties and calls realize is
> fairly easy. Maybe we can have QAPI support for this so that you can
> request generation of the wrapper function in the schema. Then you would
> just have to set the pointer to it in .class_init.

Agreed.

> 
> > In other words, I'd prefer to have compatibility layer(s) that
> > would make the same implementation work with the both old-style
> > and new-style APIs for creating devices.  Maybe this would
> > involve generating QAPI code from QOM/qdev property lists, and/or
> > generating qdev property registration code from the QAPI schema,
> > I don't know.
> > 
> > Providing a compatibility layer would also help us be more
> > confident that there are no gaps in the abstractions provided by
> > the two systems (QOM properties and QAPI schema) that we still
> > need to address.
> 
> qdev properties could be more useful to generate at least a skeleton
> schema from than generic QOM properties. But there will still be large
> parts that a human needs to do.

I agree there's lot of manual work to do.  My proposal is doing
the manual work during in the conversion of dynamic properties to
field properties.  Conversion from field properties to the new
API then would be automated.

> 
> My concern with the compatibility layer is just that it could happen
> more easily that we're stuck with it forever.

I hear you.

I'm a pessimistic regarding this.  I believe we _will_ be stuck
in the transition forever (or for so long that it will feel like
forever).  But with a good transition/compatibility layer, that
transition stage will be less painful.

I also believe that writing compatibility layer(s) would be less
work than writing and reviewing patches doing manual conversion
of device code directly to the new API.

-- 
Eduardo