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 <gr...@kaod.org>
>>>
>>> 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 <gr...@kaod.org>
>>> Message-Id:
>>> <148103887228.22326.478406873609299999.st...@bahia.lab.toulouse-stg.fr.ibm.com>
>>> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
>>> ---
>>> 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 <pa...@linux.vnet.ibm.com>
>>
>>
>> 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/qemu-devel@nongnu.org/msg416670.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg416985.html
>
