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

[Cornelia Huck]

On Mon, 05 Dec 2016 16:21:22 +0100
Greg Kurz <address@hidden> 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 <address@hidden>
> ---
> 
> 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.