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