Re: [Qemu-devel] [PATCH 1/3] qdev: fix the order compat and global properties are applied

[Halil Pasic]

On 07/12/2017 08:29 PM, Eduardo Habkost wrote:
> On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote:
>>
>>
>> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
>>> From: Greg Kurz <address@hidden>
>>>
>>> The current code recursively applies global properties from child up to
>>> parent types. This can cause properties passed with the -global option to
>>> be silently overridden by internal compat properties.
>>>
>>> This is exactly what happens with virtio-*-pci drivers since commit:
>>>
>>> "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour"
>>>
>>> Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6
>>> machine types because the internal virtio-pci.disable-modern=on compat
>>> property always prevail.
>>>
>>> This patch fixes the issue by reversing the logic: we now go through the
>>> global property list and, for each property, we check if it is applicable
>>> to the device.
>>>
>>> This result in compat properties being applied first, in the order they
>>> appear in the HW_COMPAT_* macros, followed by global properties, in they
>>> order appear on the command line.
>>>
>>> Signed-off-by: Greg Kurz <address@hidden>
>>> Message-Id: <address@hidden>
>>> Signed-off-by: Eduardo Habkost <address@hidden>
>>> ---
>>>  hw/core/qdev-properties.c | 15 ++-------------
>>>  1 file changed, 2 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index f11d578..41cca9d 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -1148,8 +1148,7 @@ int qdev_prop_check_globals(void)
>>>      return ret;
>>>  }
>>>
>>> -static void qdev_prop_set_globals_for_type(DeviceState *dev,
>>> -                                           const char *typename)
>>> +void qdev_prop_set_globals(DeviceState *dev)
>>>  {
>>>      GList *l;
>>>
>>> @@ -1157,7 +1156,7 @@ static void 
>>> qdev_prop_set_globals_for_type(DeviceState *dev,
>>>          GlobalProperty *prop = l->data;
>>>          Error *err = NULL;
>>>
>>> -        if (strcmp(typename, prop->driver) != 0) {
>>> +        if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
>>>              continue;
>>>          }
>>>          prop->used = true;
>>> @@ -1175,16 +1174,6 @@ static void 
>>> qdev_prop_set_globals_for_type(DeviceState *dev,
>>>      }
>>>  }
>>>
>>> -void qdev_prop_set_globals(DeviceState *dev)
>>> -{
>>> -    ObjectClass *class = object_get_class(OBJECT(dev));
>>> -
>>> -    do {
>>> -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
>>> -        class = object_class_get_parent(class);
>>> -    } while (class);
>>> -}
>>> -
>>>  /* --- 64bit unsigned int 'size' type --- */
>>>
>>>  static void get_size(Object *obj, Visitor *v, const char *name, void 
>>> *opaque,
>>>
>>
>> Code looks good to me. Given that high profile people are happy with the
>> patch I won't object on the behavior aspect which I don't understand fully.
>> Thus:
>>
>> Reviewed-by: Halil Pasic <address@hidden>
>>
>>
>> Now a couple of questions for keeping  my clear conscience:
>> * What did we test? Since this patch is fixing a problem it
>> changes behavior. Did we test if there is something that breaks?
>>
>> * The previous version seems to establish a (somewhat strange)
>> precedence for the case the same device property (storage object)
>> is set via multiple super-classes (e.g. set both by parent and
>> parents parent). This seems to have at least been possible,
>> and technically it still is I guess. Now instead of most general
>> (super class) wins we have last added property wins. I assume it
>> isn't a problem, because we don't have something obscure like that.
>> Or am I wrong? This obviously connects to the first question.
>> (By the way, most specialized wins would not have been that
>> surprising given how inheritance and OO usually works. My assumption
>> that nobody used this obscure mechanism is largely based on it's
>> strangeness).
> 
> Note that we are not changing the behavior when the classes
> themselves set different defaults.  Subclasses are still free to
> override defaults set by superclasses inside QEMU code, and they
> will be unaffected by this series.  What we are changing here are
> the semantics of the -global command-line option when applied to
> superclasses.

I was not referring to this.

> 
> The main sources of global properties we have are:
> * MachineClass::compat_props> * -global command-line option

I was thinking about these two.

> * -cpu command-line option
> 
> The behavior on the compat_props case was addressed by the hack
> in commit 0bcba41f "machine: Convert abstract typename on
> compat_props to subclass names".  This means compat_props was
> already following the order in which properties were registered.

I disagree. Commit 0bcba41f affects only *abstract* classes. So
your statement is true if a non-abstract class inheriting form
device can only have abstract ancestor classes. I'm not aware
such rule exists in QEMU, and according to your reply to my comment
on patch 2 there are even people using non-abstract superclasses
for devices.

Since I don't tend to trust myself with constructing proofs
for such stuff in my head, I've tried out my hypothesis before
making my review.

This is the patch I used to verify my hypothesis:
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 41ca666..d524cd0 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -445,6 +445,10 @@ static const TypeInfo ccw_machine_info = {
             .property = "max_revision",\
             .value    = "0",\
         },{\
+            .driver   = "virtio-ccw-device",\
+            .property = "max_revision",\
+            .value    = "1",\
+        },{\
             .driver   = "virtio-balloon-ccw",\
             .property = "max_revision",\
             .value    = "0",\
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e18fd26..6992697 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1666,7 +1666,7 @@ static const TypeInfo virtio_ccw_device_info = {
     .instance_size = sizeof(VirtioCcwDevice),
     .class_init = virtio_ccw_device_class_init,
     .class_size = sizeof(VirtIOCCWDeviceClass),
-    .abstract = true,
+    .abstract = false,
 };
 
 /* virtio-ccw-bus */

Unfortunately it's virtio-ccw because I'm most familiar with that,
and I knew immediately how can I construct the situation I have in
mind there.

What we observe as the effect of this patch before your patch
both my virtio-ccw-blk and virtio-ccw-net were revision 1
when running a s390-ccw-virtio-2.4 (more general takes precedence).
After your patch series, since  virtio-ccw-net is further down in
the list, it ended up being revision 0 (virtio-ccw-blk remained
1 as my change was inserted after the property for virtio-ccw-blk
but before the property for virtio-ccw-net (in the array of
compat_prpos).

Do you agree, or should I re-check my experiment and maybe also
look for some example you can run on amd64.

> In this case there should be no behavior change, and we have
> something to test: check if the original bug[1] (where -global
> was unable to override compat_props) is still fixed.
> 
> However, the behavior of -global will change if the user
> specifies command-line options that contradict each other.  I
> don't believe users rely on that behavior, and the old behavior
> was a bug and not a feature.  In this case we can test it, but
> the behavior change is intentional.

I don't think old behavior was sane, that's why I gave my r-b
without insisting on the for me open questions.

Btw. I would not call that contradicting. But it certainly
is not something our users should rely on because (as we concluded
in the discussion at patch 2), using -global for a superclass is
not documented.

> 
> [1] https://www.mail-archive.com/address@hidden/msg416670.html
>     https://www.mail-archive.com/address@hidden/msg416985.html
>