[Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

The current code recursively applies global properties from child up to
parent. So, if you have:

-global virtio-pci.disable-modern=on
-global virtio-blk-pci.disable-modern=off

Then the default value of disable-modern for a virtio-blk-pci device is on,
which looks wrong from an OOP perspective.

This patch reverses the logic, so that a child property always prevail.

This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
would silently override global properties passed on the command line for
virtio subtypes.

Signed-off-by: Greg Kurz <groug@kaod.org>
---

AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
tag to pass global properties to QEMU. This patch ensures that XML files
used with older machine types remain valid with newer versions of QEMU.

FWIW I guess it could help to have this fix in 2.8, and also probably in
2.7.1.

Please advise.

 hw/core/qdev-properties.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 2a8276806721..1345f489d6b1 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1119,11 +1119,20 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
 void qdev_prop_set_globals(DeviceState *dev)
 {
     ObjectClass *class = object_get_class(OBJECT(dev));
+    GSList *class_list = NULL;
 
     do {
-        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
+        class_list = g_slist_prepend(class_list, class);
         class = object_class_get_parent(class);
     } while (class);
+
+    do {
+        GSList *head = class_list;
+
+        qdev_prop_set_globals_for_type(dev, object_class_get_name(head->data));
+        class_list = g_slist_next(head);
+        g_slist_free_1(head);
+    } while (class_list);
 }
 
 /* --- 64bit unsigned int 'size' type --- */

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Cornelia Huck]

On Mon, 05 Dec 2016 16:21:22 +0100
Greg Kurz <groug@kaod.org> wrote:

> The current code recursively applies global properties from child up to
> parent. So, if you have:
> 
> -global virtio-pci.disable-modern=on
> -global virtio-blk-pci.disable-modern=off
> 
> Then the default value of disable-modern for a virtio-blk-pci device is on,
> which looks wrong from an OOP perspective.
> 
> This patch reverses the logic, so that a child property always prevail.

This sounds reasonable...

> 
> This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> would silently override global properties passed on the command line for
> virtio subtypes.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> tag to pass global properties to QEMU. This patch ensures that XML files
> used with older machine types remain valid with newer versions of QEMU.
> 
> FWIW I guess it could help to have this fix in 2.8, and also probably in
> 2.7.1.

...but I'm a bit worried about doing that change this late in the
cycle, as we may introduce subtle changes for other configurations. At
the very least, we should look over the existing backwards compat
properties (I'll look at those I'm familiar with).

> 
> Please advise.
> 
>  hw/core/qdev-properties.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2a8276806721..1345f489d6b1 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1119,11 +1119,20 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
>  void qdev_prop_set_globals(DeviceState *dev)
>  {
>      ObjectClass *class = object_get_class(OBJECT(dev));
> +    GSList *class_list = NULL;
> 
>      do {
> -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
> +        class_list = g_slist_prepend(class_list, class);
>          class = object_class_get_parent(class);
>      } while (class);
> +
> +    do {
> +        GSList *head = class_list;
> +
> +        qdev_prop_set_globals_for_type(dev, object_class_get_name(head->data));
> +        class_list = g_slist_next(head);
> +        g_slist_free_1(head);
> +    } while (class_list);
>  }
> 
>  /* --- 64bit unsigned int 'size' type --- */
> 

It is a bit unfortunate that we need a double loop here, but I don't
see any good alternative.

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Cornelia Huck]

On Mon, 5 Dec 2016 16:42:00 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 05 Dec 2016 16:21:22 +0100
> Greg Kurz <groug@kaod.org> wrote:

> > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > tag to pass global properties to QEMU. This patch ensures that XML files
> > used with older machine types remain valid with newer versions of QEMU.

I recall some libvirt patches floating around for this legacy/modern
stuff, but I don't know their status.

> > 
> > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > 2.7.1.
> 
> ...but I'm a bit worried about doing that change this late in the
> cycle, as we may introduce subtle changes for other configurations. At
> the very least, we should look over the existing backwards compat
> properties (I'll look at those I'm familiar with).

The s390x properties seem safe.

For virtio-pci, the ability to override extra state might become
problematic for modern devices. Although manually setting this property
is probably a patholotical case...

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

On Mon, 5 Dec 2016 17:01:28 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 5 Dec 2016 16:42:00 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Mon, 05 Dec 2016 16:21:22 +0100
> > Greg Kurz <groug@kaod.org> wrote:  
> 
> > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > > tag to pass global properties to QEMU. This patch ensures that XML files
> > > used with older machine types remain valid with newer versions of QEMU.  
> 
> I recall some libvirt patches floating around for this legacy/modern
> stuff, but I don't know their status.
> 

libvirt does some probing of disable-legacy but I could find nothing that
allows a user to explicitly choose between legacy or modern.

> > > 
> > > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > > 2.7.1.  
> > 
> > ...but I'm a bit worried about doing that change this late in the
> > cycle, as we may introduce subtle changes for other configurations. At
> > the very least, we should look over the existing backwards compat
> > properties (I'll look at those I'm familiar with).  
> 
> The s390x properties seem safe.
> 
> For virtio-pci, the ability to override extra state might become
> problematic for modern devices. Although manually setting this property
> is probably a patholotical case...
> 

I agree that migrating a 2.6 machine type with disable-modern=off being
set through -global for a given subtype, from QEMU 2.6 to QEMU 2.8 (or 2.7)
is probably a corner case :)

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Eduardo Habkost]

On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:
> On Mon, 05 Dec 2016 16:21:22 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > The current code recursively applies global properties from child up to
> > parent. So, if you have:
> > 
> > -global virtio-pci.disable-modern=on
> > -global virtio-blk-pci.disable-modern=off
> > 
> > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > which looks wrong from an OOP perspective.
> > 
> > This patch reverses the logic, so that a child property always prevail.
> 
> This sounds reasonable...
> 
> > 
> > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > would silently override global properties passed on the command line for
> > virtio subtypes.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > tag to pass global properties to QEMU. This patch ensures that XML files
> > used with older machine types remain valid with newer versions of QEMU.
> > 
> > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > 2.7.1.
> 
> ...but I'm a bit worried about doing that change this late in the
> cycle, as we may introduce subtle changes for other configurations. At
> the very least, we should look over the existing backwards compat
> properties (I'll look at those I'm familiar with).

This patch would change the behavior for:
 -global virtio-blk-pci.disable-modern=on
 -global virtio-pci.disable-modern=off

And I am not sure the new behavior would be correct. Shouldn't we
apply the properties in the order specified in the command-line?

On either case, changing the semantics of the command-line can
break existing configurations. Let's do it more carefully in the
2.9 cycle, and fix the existing bug by changing the HW_COMPAT_*
macros?

> 
> > 
> > Please advise.
> > 
> >  hw/core/qdev-properties.c |   11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 2a8276806721..1345f489d6b1 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1119,11 +1119,20 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> >  void qdev_prop_set_globals(DeviceState *dev)
> >  {
> >      ObjectClass *class = object_get_class(OBJECT(dev));
> > +    GSList *class_list = NULL;
> > 
> >      do {
> > -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
> > +        class_list = g_slist_prepend(class_list, class);
> >          class = object_class_get_parent(class);
> >      } while (class);
> > +
> > +    do {
> > +        GSList *head = class_list;
> > +
> > +        qdev_prop_set_globals_for_type(dev, object_class_get_name(head->data));
> > +        class_list = g_slist_next(head);
> > +        g_slist_free_1(head);
> > +    } while (class_list);
> >  }
> > 
> >  /* --- 64bit unsigned int 'size' type --- */
> > 
> 
> It is a bit unfortunate that we need a double loop here, but I don't
> see any good alternative.
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Cornelia Huck]

On Mon, 5 Dec 2016 14:48:29 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:
> > On Mon, 05 Dec 2016 16:21:22 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > The current code recursively applies global properties from child up to
> > > parent. So, if you have:
> > > 
> > > -global virtio-pci.disable-modern=on
> > > -global virtio-blk-pci.disable-modern=off
> > > 
> > > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > > which looks wrong from an OOP perspective.
> > > 
> > > This patch reverses the logic, so that a child property always prevail.
> > 
> > This sounds reasonable...
> > 
> > > 
> > > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > > would silently override global properties passed on the command line for
> > > virtio subtypes.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 
> > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > > tag to pass global properties to QEMU. This patch ensures that XML files
> > > used with older machine types remain valid with newer versions of QEMU.
> > > 
> > > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > > 2.7.1.
> > 
> > ...but I'm a bit worried about doing that change this late in the
> > cycle, as we may introduce subtle changes for other configurations. At
> > the very least, we should look over the existing backwards compat
> > properties (I'll look at those I'm familiar with).
> 
> This patch would change the behavior for:
>  -global virtio-blk-pci.disable-modern=on
>  -global virtio-pci.disable-modern=off
> 
> And I am not sure the new behavior would be correct. Shouldn't we
> apply the properties in the order specified in the command-line?

Probably; but how should this interact with compat props?

> 
> On either case, changing the semantics of the command-line can
> break existing configurations. Let's do it more carefully in the
> 2.9 cycle, and fix the existing bug by changing the HW_COMPAT_*
> macros?

Changing the compat props is probably the best option at this point in
time. Let's take this slowly so we can come up with a reasonable
solution for 2.9.

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Eduardo Habkost]

On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote:
> On Mon, 5 Dec 2016 14:48:29 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:
> > > On Mon, 05 Dec 2016 16:21:22 +0100
> > > Greg Kurz <groug@kaod.org> wrote:
> > > 
> > > > The current code recursively applies global properties from child up to
> > > > parent. So, if you have:
> > > > 
> > > > -global virtio-pci.disable-modern=on
> > > > -global virtio-blk-pci.disable-modern=off
> > > > 
> > > > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > > > which looks wrong from an OOP perspective.
> > > > 
> > > > This patch reverses the logic, so that a child property always prevail.
> > > 
> > > This sounds reasonable...
> > > 
> > > > 
> > > > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > > > would silently override global properties passed on the command line for
> > > > virtio subtypes.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > > 
> > > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > > > tag to pass global properties to QEMU. This patch ensures that XML files
> > > > used with older machine types remain valid with newer versions of QEMU.
> > > > 
> > > > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > > > 2.7.1.
> > > 
> > > ...but I'm a bit worried about doing that change this late in the
> > > cycle, as we may introduce subtle changes for other configurations. At
> > > the very least, we should look over the existing backwards compat
> > > properties (I'll look at those I'm familiar with).
> > 
> > This patch would change the behavior for:
> >  -global virtio-blk-pci.disable-modern=on
> >  -global virtio-pci.disable-modern=off
> > 
> > And I am not sure the new behavior would be correct. Shouldn't we
> > apply the properties in the order specified in the command-line?
> 
> Probably; but how should this interact with compat props?

compat props should be always applied in the order they appear.
-global should always be applied after compat props.

So, it looks like we have two additional reasons to just follow
the order the global properties were registered.

> 
> > 
> > On either case, changing the semantics of the command-line can
> > break existing configurations. Let's do it more carefully in the
> > 2.9 cycle, and fix the existing bug by changing the HW_COMPAT_*
> > macros?
> 
> Changing the compat props is probably the best option at this point in
> time. Let's take this slowly so we can come up with a reasonable
> solution for 2.9.

Agreed.

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Halil Pasic]

On 12/05/2016 06:41 PM, Eduardo Habkost wrote:
> On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote:
>> > On Mon, 5 Dec 2016 14:48:29 -0200
>> > Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > 
>>> > > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:
>>>> > > > On Mon, 05 Dec 2016 16:21:22 +0100
>>>> > > > Greg Kurz <groug@kaod.org> wrote:
>>>> > > > 
>>>>> > > > > The current code recursively applies global properties from child up to
>>>>> > > > > parent. So, if you have:
>>>>> > > > > 
>>>>> > > > > -global virtio-pci.disable-modern=on
>>>>> > > > > -global virtio-blk-pci.disable-modern=off
>>>>> > > > > 
>>>>> > > > > Then the default value of disable-modern for a virtio-blk-pci device is on,
>>>>> > > > > which looks wrong from an OOP perspective.
>>>>> > > > > 
>>>>> > > > > This patch reverses the logic, so that a child property always prevail.
>>>> > > > 
>>>> > > > This sounds reasonable...
>>>> > > > 
>>>>> > > > > 
>>>>> > > > > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
>>>>> > > > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
>>>>> > > > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
>>>>> > > > > would silently override global properties passed on the command line for
>>>>> > > > > virtio subtypes.
>>>>> > > > > 
>>>>> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>> > > > > ---
>>>>> > > > > 
>>>>> > > > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
>>>>> > > > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
>>>>> > > > > tag to pass global properties to QEMU. This patch ensures that XML files
>>>>> > > > > used with older machine types remain valid with newer versions of QEMU.
>>>>> > > > > 
>>>>> > > > > FWIW I guess it could help to have this fix in 2.8, and also probably in
>>>>> > > > > 2.7.1.
>>>> > > > 
>>>> > > > ...but I'm a bit worried about doing that change this late in the
>>>> > > > cycle, as we may introduce subtle changes for other configurations. At
>>>> > > > the very least, we should look over the existing backwards compat
>>>> > > > properties (I'll look at those I'm familiar with).
>>> > > 
>>> > > This patch would change the behavior for:
>>> > >  -global virtio-blk-pci.disable-modern=on
>>> > >  -global virtio-pci.disable-modern=off
>>> > > 
>>> > > And I am not sure the new behavior would be correct. Shouldn't we
>>> > > apply the properties in the order specified in the command-line?
>> > 
>> > Probably; but how should this interact with compat props?
> compat props should be always applied in the order they appear.
> -global should always be applied after compat props.
> 
> So, it looks like we have two additional reasons to just follow
> the order the global properties were registered.
> 

How about a docs update? 

Given the current doc:
"""
-global driver.prop=value
-global driver=driver,property=property,value=value
    Set default value of driver's property prop to value, e.g.:

         qemu-system-i386 -global ide-drive.physical_block_size=4096 -drive
file=file,if=ide,index=0,media=disk

    In particular, you can use this to set driver properties for devices which
    are created automatically by the machine model. To create a device which is
    not created automatically and set properties on it, use -device.

    -global driver.prop=value is shorthand for -global driver=driver,property=prop,
value=value. The longhand syntax works even when
    driver contains a dot. 
"""
I think this OOP argument, which I do not completely understand,
is not the right direction.

Yet I do not think the current state of documentation gives a
definitive answer on what behavior should take place in the
scenarios described above.

Maybe it's my mistake, but I did not find a statement about
the order in which global properties are to be applied
(please point me to it if I've missed it).

Another problem with the doc (IMHO) is that it's not
really defined what a driver is. So I can't even tell if
-global virtio-pci.disable-modern=off is even legit on
the command line.

Regarding the order compat props. applied before command line it
makes a lot of sense to me with the documentation stating "In 
particular, you can use this to set driver properties for devices which
are created automatically by the machine model."

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

On Mon, 5 Dec 2016 19:14:40 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/05/2016 06:41 PM, Eduardo Habkost wrote:
> > On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote:  
> >> > On Mon, 5 Dec 2016 14:48:29 -0200
> >> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> >   
> >>> > > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:  
> >>>> > > > On Mon, 05 Dec 2016 16:21:22 +0100
> >>>> > > > Greg Kurz <groug@kaod.org> wrote:
> >>>> > > >   
> >>>>> > > > > The current code recursively applies global properties from child up to
> >>>>> > > > > parent. So, if you have:
> >>>>> > > > > 
> >>>>> > > > > -global virtio-pci.disable-modern=on
> >>>>> > > > > -global virtio-blk-pci.disable-modern=off
> >>>>> > > > > 
> >>>>> > > > > Then the default value of disable-modern for a virtio-blk-pci device is on,
> >>>>> > > > > which looks wrong from an OOP perspective.
> >>>>> > > > > 
> >>>>> > > > > This patch reverses the logic, so that a child property always prevail.  
> >>>> > > > 
> >>>> > > > This sounds reasonable...
> >>>> > > >   
> >>>>> > > > > 
> >>>>> > > > > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> >>>>> > > > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> >>>>> > > > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> >>>>> > > > > would silently override global properties passed on the command line for
> >>>>> > > > > virtio subtypes.
> >>>>> > > > > 
> >>>>> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> >>>>> > > > > ---
> >>>>> > > > > 
> >>>>> > > > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> >>>>> > > > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> >>>>> > > > > tag to pass global properties to QEMU. This patch ensures that XML files
> >>>>> > > > > used with older machine types remain valid with newer versions of QEMU.
> >>>>> > > > > 
> >>>>> > > > > FWIW I guess it could help to have this fix in 2.8, and also probably in
> >>>>> > > > > 2.7.1.  
> >>>> > > > 
> >>>> > > > ...but I'm a bit worried about doing that change this late in the
> >>>> > > > cycle, as we may introduce subtle changes for other configurations. At
> >>>> > > > the very least, we should look over the existing backwards compat
> >>>> > > > properties (I'll look at those I'm familiar with).  
> >>> > > 
> >>> > > This patch would change the behavior for:
> >>> > >  -global virtio-blk-pci.disable-modern=on
> >>> > >  -global virtio-pci.disable-modern=off
> >>> > > 
> >>> > > And I am not sure the new behavior would be correct. Shouldn't we
> >>> > > apply the properties in the order specified in the command-line?  
> >> > 
> >> > Probably; but how should this interact with compat props?  
> > compat props should be always applied in the order they appear.
> > -global should always be applied after compat props.
> > 
> > So, it looks like we have two additional reasons to just follow
> > the order the global properties were registered.
> >   
> 
> How about a docs update? 
> 
> Given the current doc:
> """
> -global driver.prop=value
> -global driver=driver,property=property,value=value
>     Set default value of driver's property prop to value, e.g.:
> 
>          qemu-system-i386 -global ide-drive.physical_block_size=4096 -drive
> file=file,if=ide,index=0,media=disk
> 
>     In particular, you can use this to set driver properties for devices which
>     are created automatically by the machine model. To create a device which is
>     not created automatically and set properties on it, use -device.
> 
>     -global driver.prop=value is shorthand for -global driver=driver,property=prop,
> value=value. The longhand syntax works even when
>     driver contains a dot. 
> """
> I think this OOP argument, which I do not completely understand,

With the current code, properties from the parent classes implicitly
prevail and this has nothing to do with command line order, or order
of appearance in HW_COMPAT_*.

From an OOP perspective, we usually expect child classes to override
parent classes behavior, not the contrary.

> is not the right direction.
> 
> Yet I do not think the current state of documentation gives a
> definitive answer on what behavior should take place in the
> scenarios described above.
> 

True, the documentation doesn't mention anything about the QEMU
object model. And the user cannot even think that virtio-pci
exists and is the parent of virtio-blk-pci...

> Maybe it's my mistake, but I did not find a statement about
> the order in which global properties are to be applied
> (please point me to it if I've missed it).
> 

True, so this should be the implicit command line order.

> Another problem with the doc (IMHO) is that it's not
> really defined what a driver is. So I can't even tell if
> -global virtio-pci.disable-modern=off is even legit on
> the command line.
> 

True, virtio-pci.disable-modern is an internal compat thing and
it silentyl overrides virtio-blk-pci.disable-modern which is a
legit option. Passing virtio-pci.disable-modern to the command
line is just messing with undocumented internals... and should
probably be disallowed.

> Regarding the order compat props. applied before command line it
> makes a lot of sense to me with the documentation stating "In 
> particular, you can use this to set driver properties for devices which
> are created automatically by the machine model."
> 

I agree. So, as written several times in this thread, the only thing
we can do now is changing the virtio-pci compat props to be applied
to each subtype, so that they don't silently undo -global.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Halil Pasic]

On 12/06/2016 10:11 AM, Greg Kurz wrote:
>> Given the current doc:
>> > """
>> > -global driver.prop=value
>> > -global driver=driver,property=property,value=value
>> >     Set default value of driver's property prop to value, e.g.:
>> > 
>> >          qemu-system-i386 -global ide-drive.physical_block_size=4096 -drive
>> > file=file,if=ide,index=0,media=disk
>> > 
>> >     In particular, you can use this to set driver properties for devices which
>> >     are created automatically by the machine model. To create a device which is
>> >     not created automatically and set properties on it, use -device.
>> > 
>> >     -global driver.prop=value is shorthand for -global driver=driver,property=prop,
>> > value=value. The longhand syntax works even when
>> >     driver contains a dot. 
>> > """
>> > I think this OOP argument, which I do not completely understand,
> With the current code, properties from the parent classes implicitly
> prevail and this has nothing to do with command line order, or order
> of appearance in HW_COMPAT_*.
>

Yeah and IMHO this is exactly the problem :).
 
> From an OOP perspective, we usually expect child classes to override
> parent classes behavior, not the contrary.
> 


I know, but the question is if it is the right analogy. The point is (as
you yourself already stated in a follow up email) the semantic of global
properties can be very well defined as for each instance of the class X
set property P to value V (that is imperatively). Now if it is possible
that same data exposed by properties is set more than once (e.g. via
parent and via child) and we want to remain deterministic we need to say
which write is going to win by being the last one.

Another option would be to say define this in a functional manner and
say the value of a property is going to be V unless ... ( here we need
to state that child takes precedence over parent and bring in command
line in in some way too -- where I do not know if parent command line
should take precedence over compat property on the child or the other
way around).

Because IMHO the first option is not consistent with the doc I'm in
favor of that, but then it has not much to do with overriding behavior.

Thanks for the explanation. Given your other email I think we are in
agreement now. I still think a bit more documentation (not necessarily
user documentation) would not hurt, as IMHO the OOP intuition you stated
is a valid one too (not my favored one though) and thus it would not
hurt to have the design decision captured in natural language too.

Cheers,
Halil

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

On Mon, 5 Dec 2016 15:41:30 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote:
> > On Mon, 5 Dec 2016 14:48:29 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:  
> > > > On Mon, 05 Dec 2016 16:21:22 +0100
> > > > Greg Kurz <groug@kaod.org> wrote:
> > > >   
> > > > > The current code recursively applies global properties from child up to
> > > > > parent. So, if you have:
> > > > > 
> > > > > -global virtio-pci.disable-modern=on
> > > > > -global virtio-blk-pci.disable-modern=off
> > > > > 
> > > > > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > > > > which looks wrong from an OOP perspective.
> > > > > 
> > > > > This patch reverses the logic, so that a child property always prevail.  
> > > > 
> > > > This sounds reasonable...
> > > >   
> > > > > 
> > > > > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > > > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > > > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > > > > would silently override global properties passed on the command line for
> > > > > virtio subtypes.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > > 
> > > > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > > > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > > > > tag to pass global properties to QEMU. This patch ensures that XML files
> > > > > used with older machine types remain valid with newer versions of QEMU.
> > > > > 
> > > > > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > > > > 2.7.1.  
> > > > 
> > > > ...but I'm a bit worried about doing that change this late in the
> > > > cycle, as we may introduce subtle changes for other configurations. At
> > > > the very least, we should look over the existing backwards compat
> > > > properties (I'll look at those I'm familiar with).  
> > > 
> > > This patch would change the behavior for:
> > >  -global virtio-blk-pci.disable-modern=on
> > >  -global virtio-pci.disable-modern=off
> > > 
> > > And I am not sure the new behavior would be correct. Shouldn't we
> > > apply the properties in the order specified in the command-line?  
> > 
> > Probably; but how should this interact with compat props?  
> 
> compat props should be always applied in the order they appear.
> -global should always be applied after compat props.
> 

This is actually the way they're being registered to the global_props
static list: compat props as they appear in HW_COMPAT_* and then -global
as they appear on the command line.

> So, it looks like we have two additional reasons to just follow
> the order the global properties were registered.
> 

Thinking again, maybe we just need to reverse the logic in another
way: go through global_props and apply the property if the device
can be casted to the corresponding class (i.e. object_class_dynamic_cast()
!= NULL). I'll try that.

> >   
> > > 
> > > On either case, changing the semantics of the command-line can
> > > break existing configurations. Let's do it more carefully in the
> > > 2.9 cycle, and fix the existing bug by changing the HW_COMPAT_*
> > > macros?  
> > 
> > Changing the compat props is probably the best option at this point in
> > time. Let's take this slowly so we can come up with a reasonable
> > solution for 2.9.  
> 
> Agreed.
> 

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Halil Pasic]

On 12/06/2016 10:30 AM, Greg Kurz wrote:
> On Mon, 5 Dec 2016 15:41:30 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> > On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote:
>>> > > On Mon, 5 Dec 2016 14:48:29 -0200
>>> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> > >   
>>>> > > > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:  
>>>>> > > > > On Mon, 05 Dec 2016 16:21:22 +0100
>>>>> > > > > Greg Kurz <groug@kaod.org> wrote:
>>>>> > > > >   
>>>>>> > > > > > The current code recursively applies global properties from child up to
>>>>>> > > > > > parent. So, if you have:
>>>>>> > > > > > 
>>>>>> > > > > > -global virtio-pci.disable-modern=on
>>>>>> > > > > > -global virtio-blk-pci.disable-modern=off
>>>>>> > > > > > 
>>>>>> > > > > > Then the default value of disable-modern for a virtio-blk-pci device is on,
>>>>>> > > > > > which looks wrong from an OOP perspective.
>>>>>> > > > > > 
>>>>>> > > > > > This patch reverses the logic, so that a child property always prevail.  
[...]
>> > compat props should be always applied in the order they appear.
>> > -global should always be applied after compat props.
>> > 
> This is actually the way they're being registered to the global_props
> static list: compat props as they appear in HW_COMPAT_* and then -global
> as they appear on the command line.
> 
>> > So, it looks like we have two additional reasons to just follow
>> > the order the global properties were registered.
>> > 
> Thinking again, maybe we just need to reverse the logic in another
> way: go through global_props and apply the property if the device
> can be casted to the corresponding class (i.e. object_class_dynamic_cast()
> != NULL). I'll try that.
> 

IMHO this is the right thing to do (and would result in the
exactly behavior outlined by Eduardo -- compat props as they
applied appear and then command line props as they appear, what
means inverse ordering in terms of overriding regarding the
result).

Halil

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

On Mon, 5 Dec 2016 14:48:29 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote:
> > On Mon, 05 Dec 2016 16:21:22 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> > > The current code recursively applies global properties from child up to
> > > parent. So, if you have:
> > > 
> > > -global virtio-pci.disable-modern=on
> > > -global virtio-blk-pci.disable-modern=off
> > > 
> > > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > > which looks wrong from an OOP perspective.
> > > 
> > > This patch reverses the logic, so that a child property always prevail.  
> > 
> > This sounds reasonable...
> >   
> > > 
> > > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > > would silently override global properties passed on the command line for
> > > virtio subtypes.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 
> > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > > tag to pass global properties to QEMU. This patch ensures that XML files
> > > used with older machine types remain valid with newer versions of QEMU.
> > > 
> > > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > > 2.7.1.  
> > 
> > ...but I'm a bit worried about doing that change this late in the
> > cycle, as we may introduce subtle changes for other configurations. At
> > the very least, we should look over the existing backwards compat
> > properties (I'll look at those I'm familiar with).  
> 
> This patch would change the behavior for:
>  -global virtio-blk-pci.disable-modern=on
>  -global virtio-pci.disable-modern=off
> 

Yes.

> And I am not sure the new behavior would be correct. Shouldn't we
> apply the properties in the order specified in the command-line?
> 

I would generally agree and this is what actually happens when setting
properties for a given class, but here we have the extra dimension of
class inheritance. Enforcing the command line ordering would require
to mix the setting of parent and child properties... maybe doable but
definitely a bigger change in the existing logic IMHO.

> On either case, changing the semantics of the command-line can
> break existing configurations. Let's do it more carefully in the
> 2.9 cycle, and fix the existing bug by changing the HW_COMPAT_*
> macros?
> 

That would be another possibility which is less intrusive indeed: change
the HW_COMPAT_2.6 macro to set properties for each individual virtio-pci
subtypes.

> >   
> > > 
> > > Please advise.
> > > 
> > >  hw/core/qdev-properties.c |   11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > index 2a8276806721..1345f489d6b1 100644
> > > --- a/hw/core/qdev-properties.c
> > > +++ b/hw/core/qdev-properties.c
> > > @@ -1119,11 +1119,20 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> > >  void qdev_prop_set_globals(DeviceState *dev)
> > >  {
> > >      ObjectClass *class = object_get_class(OBJECT(dev));
> > > +    GSList *class_list = NULL;
> > > 
> > >      do {
> > > -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
> > > +        class_list = g_slist_prepend(class_list, class);
> > >          class = object_class_get_parent(class);
> > >      } while (class);
> > > +
> > > +    do {
> > > +        GSList *head = class_list;
> > > +
> > > +        qdev_prop_set_globals_for_type(dev, object_class_get_name(head->data));
> > > +        class_list = g_slist_next(head);
> > > +        g_slist_free_1(head);
> > > +    } while (class_list);
> > >  }
> > > 
> > >  /* --- 64bit unsigned int 'size' type --- */
> > >   
> > 
> > It is a bit unfortunate that we need a double loop here, but I don't
> > see any good alternative.
> > 
> >   
> 

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

On Mon, 5 Dec 2016 16:42:00 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 05 Dec 2016 16:21:22 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > The current code recursively applies global properties from child up to
> > parent. So, if you have:
> > 
> > -global virtio-pci.disable-modern=on
> > -global virtio-blk-pci.disable-modern=off
> > 
> > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > which looks wrong from an OOP perspective.
> > 
> > This patch reverses the logic, so that a child property always prevail.  
> 
> This sounds reasonable...
> 
> > 
> > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > would silently override global properties passed on the command line for
> > virtio subtypes.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > tag to pass global properties to QEMU. This patch ensures that XML files
> > used with older machine types remain valid with newer versions of QEMU.
> > 
> > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > 2.7.1.  
> 
> ...but I'm a bit worried about doing that change this late in the
> cycle, as we may introduce subtle changes for other configurations. At
> the very least, we should look over the existing backwards compat
> properties (I'll look at those I'm familiar with).
> 

Yeah, it's unfortunate I encountered the issue this late... I usually
pass properties to -device directly. :-\ Maybe we should set global
options per virtio-pci subtype as you suggested on IRC ?

Note: sorry for the late answer but I've just received all the mails...

> > 
> > Please advise.
> > 
> >  hw/core/qdev-properties.c |   11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 2a8276806721..1345f489d6b1 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1119,11 +1119,20 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
> >  void qdev_prop_set_globals(DeviceState *dev)
> >  {
> >      ObjectClass *class = object_get_class(OBJECT(dev));
> > +    GSList *class_list = NULL;
> > 
> >      do {
> > -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
> > +        class_list = g_slist_prepend(class_list, class);
> >          class = object_class_get_parent(class);
> >      } while (class);
> > +
> > +    do {
> > +        GSList *head = class_list;
> > +
> > +        qdev_prop_set_globals_for_type(dev, object_class_get_name(head->data));
> > +        class_list = g_slist_next(head);
> > +        g_slist_free_1(head);
> > +    } while (class_list);
> >  }
> > 
> >  /* --- 64bit unsigned int 'size' type --- */
> >   
> 
> It is a bit unfortunate that we need a double loop here, but I don't
> see any good alternative.
> 

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Daniel P. Berrange]

On Mon, Dec 05, 2016 at 04:21:22PM +0100, Greg Kurz wrote:
> The current code recursively applies global properties from child up to
> parent. So, if you have:
> 
> -global virtio-pci.disable-modern=on
> -global virtio-blk-pci.disable-modern=off
> 
> Then the default value of disable-modern for a virtio-blk-pci device is on,
> which looks wrong from an OOP perspective.
> 
> This patch reverses the logic, so that a child property always prevail.
> 
> This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> would silently override global properties passed on the command line for
> virtio subtypes.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> tag to pass global properties to QEMU. This patch ensures that XML files
> used with older machine types remain valid with newer versions of QEMU.

FYI, we explicitly do *not* guarantee any kind of stability/compatibility
if you use  qemu:commandline. Users should expect it to break when they
upgrade QEMU and/or libvirt.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On Mon, Dec 05, 2016 at 04:21:22PM +0100, Greg Kurz wrote:
> The current code recursively applies global properties from child up to
> parent. So, if you have:
> 
> -global virtio-pci.disable-modern=on
> -global virtio-blk-pci.disable-modern=off
> 
> Then the default value of disable-modern for a virtio-blk-pci device is on,
> which looks wrong from an OOP perspective.
> 
> This patch reverses the logic, so that a child property always prevail.
> 
> This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> would silently override global properties passed on the command line for
> virtio subtypes.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> tag to pass global properties to QEMU. This patch ensures that XML files
> used with older machine types remain valid with newer versions of QEMU.
> 
> FWIW I guess it could help to have this fix in 2.8, and also probably in
> 2.7.1.

Hi Greg,
I won't merge this for QEMU 2.8 because this 2.7 issue is not a 2.8
release blocker and it's too risky (good points have been raised in this
thread).

Please target -stable when consensus has been reached.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.8] qdev: apply global properties in reverse order

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On Tue, 6 Dec 2016 09:47:08 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Dec 05, 2016 at 04:21:22PM +0100, Greg Kurz wrote:
> > The current code recursively applies global properties from child up to
> > parent. So, if you have:
> > 
> > -global virtio-pci.disable-modern=on
> > -global virtio-blk-pci.disable-modern=off
> > 
> > Then the default value of disable-modern for a virtio-blk-pci device is on,
> > which looks wrong from an OOP perspective.
> > 
> > This patch reverses the logic, so that a child property always prevail.
> > 
> > This fixes a subtle bug that got introduced in 2.7 with commit "9a4c0e220d8a
> > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine types: the
> > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* properties which
> > would silently override global properties passed on the command line for
> > virtio subtypes.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > AFAIK, libvirt's XML doesn't know about modern/legacy modes for virtio
> > devices. Early adopters of virtio 1.0 had to rely on the <qemu:commandline>
> > tag to pass global properties to QEMU. This patch ensures that XML files
> > used with older machine types remain valid with newer versions of QEMU.
> > 
> > FWIW I guess it could help to have this fix in 2.8, and also probably in
> > 2.7.1.  
> 
> Hi Greg,
> I won't merge this for QEMU 2.8 because this 2.7 issue is not a 2.8
> release blocker and it's too risky (good points have been raised in this
> thread).
> 
> Please target -stable when consensus has been reached.
> 

Sure I'll just do that.

Cheers.

--
Greg

> Thanks,
> Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]