Re: [Qemu-stable] [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names

[Eduardo Habkost]

On Mon, Dec 12, 2016 at 06:13:50PM +0100, Halil Pasic wrote:
> 
> 
> On 12/12/2016 01:25 PM, Cornelia Huck wrote:
> >> +        if (oc && object_class_is_abstract(oc)) {
> >> +            /* temporary hack to make sure we will never override
>                                                ~~~~~~~~~~~~~~~~~~~~~~
> 
> I would say this is not enough to provide that guarantee. Inheriting
> from non-abstract, or something like I described below, would still
> get us in trouble. Or am I wrong? 

Yes, inheriting from non-abstract would still be broken, so this
hack is not enough for some cases. In the current tree, it won't
fix the problem for the spapr-pci-host-bridge properties.

> 
> How about s/we will never/we do not/?

I will change it.

> 
> >> +             * globals set explicitly on -global: if an abstract class
> >> +             * is on compat_props, register globals for each of their
> >> +             * subclasses instead.
> 
> I'm not sure if this is clear enough regarding multilevel inheritance.
> I would prefer "subtype" or "class derived from it".

I can rewrite it as "for all of their subtypes".

> 
> >> +             */
> > I think this should not just be a 'temporary hack'... rather document
> > this behaviour for abstract classes?
> > 
> 
> Connie, I have to disagree with you on this. I'm fine with this as a
> temporary hack provided it remedies the acute problem, but I would this
> becoming state of art.
> 
> The reason for this how abstract class is usually understood in OOP:
> it's like a normal class except you can not instantiate it.  Adding an
> extra property rule, and especially such a convoluted one, could result
> in a conflict between intuition and reality.  And it's not like we have
> no plan how to do this cleanly.
> 
> Why do I say convoluted:
> * Inheriting form abstract and from non-abstract behaves differently.
> * Theoretically we have something like non-abstract (C_3) inherits for
> abstract (AC_2) inherits from non-abstract (C_1)  inherits from abstract
> (AC_0), and we set the property P both via AC_0 and AC_2 for an instance
> of C1 we would/could end up having the same problem as we have now (via
> AC_0 taking precedence over AC_2) -- at least I think so. 

Well, we could change the code to:
1) Not check object_class_is_abstract(), and simply register the
   globals for all subtypes;
2) Call qdev_prop_set_globals_for_type() only for
   object_class_get_name(), instead of doing it for all
   parent classes in qdev_prop_set_globals().

But this sounds like a more complicated way of implementing
exactly the same behavior implemented by Greg Kurz in "qdev: fix
the order compat and global properties are applied".

> 
> I did not test it either, but it looks sane to me. Regardless of the
> comments on the comment (if you are going to consider them or not) you
> can take my r-b.

Thanks!

-- 
Eduardo