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

[Eduardo Habkost]

On Tue, Dec 06, 2016 at 04:41:12PM +0100, Greg Kurz wrote:
> 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>

FWIW, this is the behavior I would like to see. But:

I think it's too late to change the rules on 2.8. I would like to
change behavior only on 2.9, so we have time to discuss and test
it. I believe the 2.8 fix should be just changing the HW_COMPAT_*
macros to touch only the device subclasses, so we fix the
regression introduced by commit 9a4c0e220d8a without changing any
rules on how -global is handled.

> ---
>  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 5972108b2699..93a4dfeb0836 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1089,8 +1089,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;
>  
> @@ -1098,7 +1097,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;
> @@ -1120,16 +1119,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,
> 

-- 
Eduardo