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

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

[Eduardo Habkost]

Before 2.8 was released, we found a bug in the way global
properties are applied by device code.  Greg Kurz sent a fix[1],
but we decide to include a more conservative workaround[2]
because the 2.8 release was very close.

The plan was to include Greg's patch in 2.9, but we forgot to do
that.  I'm now resending: Greg's original patch; a test case to
detect the original bug; and a revert of the workaround we
included in 2.8.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg416944.html
[2] commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2
    "machine: Convert abstract typename on compat_props to subclass names"

Eduardo Habkost (2):
  test-qdev-global-props: Test global property ordering
  Revert "machine: Convert abstract typename on compat_props to subclass
    names"

Greg Kurz (1):
  qdev: fix the order compat and global properties are applied

 hw/core/machine.c              | 26 +++-----------------------
 hw/core/qdev-properties.c      | 15 ++-------------
 tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 36 deletions(-)

-- 
2.9.4

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

[Eduardo Habkost]

From: Greg Kurz <groug@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 <groug@kaod.org>
Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
Signed-off-by: Eduardo Habkost <ehabkost@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,
-- 
2.9.4

[Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Eduardo Habkost]

Test case to detect the bug fixed by commit
"qdev: fix the order compat and global properties are applied".

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 48e5b73..ef2951f 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -33,6 +33,8 @@
 #define STATIC_TYPE(obj) \
     OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
 
+#define TYPE_SUBCLASS "static_prop_subtype"
+
 #define PROP_DEFAULT 100
 
 typedef struct MyType {
@@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
     .class_init = static_prop_class_init,
 };
 
+static const TypeInfo subclass_type = {
+    .name = TYPE_SUBCLASS,
+    .parent = TYPE_STATIC_PROPS,
+};
+
 /* Test simple static property setting to default value */
 static void test_static_prop_subprocess(void)
 {
@@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
     g_test_trap_assert_stdout("");
 }
 
+/* Test if global props affecting subclasses are applied in the right order */
+static void test_subclass_global_props(void)
+{
+    MyType *mt;
+    /* Global properties must be applied in the order they were registered */
+    static GlobalProperty props[] = {
+        { TYPE_STATIC_PROPS, "prop1", "101" },
+        { TYPE_SUBCLASS,     "prop1", "102" },
+        { TYPE_SUBCLASS,     "prop2", "103" },
+        { TYPE_STATIC_PROPS, "prop2", "104" },
+        {}
+    };
+
+    qdev_prop_register_global_list(props);
+
+    mt = STATIC_TYPE(object_new(TYPE_SUBCLASS));
+    qdev_init_nofail(DEVICE(mt));
+
+    g_assert_cmpuint(mt->prop1, ==, 102);
+    g_assert_cmpuint(mt->prop2, ==, 104);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
     module_call_init(MODULE_INIT_QOM);
     type_register_static(&static_prop_type);
+    type_register_static(&subclass_type);
     type_register_static(&dynamic_prop_type);
     type_register_static(&hotplug_type);
     type_register_static(&nohotplug_type);
@@ -310,6 +340,9 @@ int main(int argc, char **argv)
     g_test_add_func("/qdev/properties/dynamic/global/nouser",
                     test_dynamic_globalprop_nouser);
 
+    g_test_add_func("/qdev/properties/global/subclass",
+                    test_subclass_global_props);
+
     g_test_run();
 
     return 0;
-- 
2.9.4

[Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Eduardo Habkost]

This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.

The bug addressed by that commit is now fixed in a better way by the
commit "qdev: fix the order compat and global properties are applied".

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ecb5552..1d10b01 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -770,18 +770,11 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
     g_free(mc->name);
 }
 
-static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
-{
-    GlobalProperty *p = opaque;
-    register_compat_prop(object_class_get_name(oc), p->property, p->value);
-}
-
 void machine_register_compat_props(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     int i;
     GlobalProperty *p;
-    ObjectClass *oc;
 
     if (!mc->compat_props) {
         return;
@@ -789,22 +782,9 @@ void machine_register_compat_props(MachineState *machine)
 
     for (i = 0; i < mc->compat_props->len; i++) {
         p = g_array_index(mc->compat_props, GlobalProperty *, i);
-        oc = object_class_by_name(p->driver);
-        if (oc && object_class_is_abstract(oc)) {
-            /* temporary hack to make sure we do not override
-             * globals set explicitly on -global: if an abstract class
-             * is on compat_props, register globals for all its
-             * non-abstract subtypes instead.
-             *
-             * This doesn't solve the problem for cases where
-             * a non-abstract typename mentioned on compat_props
-             * has subclasses, like spapr-pci-host-bridge.
-             */
-            object_class_foreach(machine_register_compat_for_subclass,
-                                 p->driver, false, p);
-        } else {
-            register_compat_prop(p->driver, p->property, p->value);
-        }
+        /* Machine compat_props must never cause errors: */
+        p->errp = &error_abort;
+        qdev_prop_register_global(p);
     }
 }
 
-- 
2.9.4

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

[Cornelia Huck]

On Mon, 10 Jul 2017 21:43:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

Some description tweaks, as we had the workaround in the meanwhile:

> From: Greg Kurz <groug@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:

s/happens/happened/
s/since/after/

> 
> "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour"
> 
> Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6

s/has/had/

> machine types because the internal virtio-pci.disable-modern=on compat
> property always prevail.

s/prevail/prevailed/

A workaround for this was included with commit 0bcba41f ("machine:
Convert abstract typename on compat_props to subclass names").

> 
> This patch fixes the issue by reversing the logic: we now go through the

s/fixes the issue/fixes the issue properly/

> 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

s/result/results/

> appear in the HW_COMPAT_* macros, followed by global properties, in they
> order appear on the command line.

s/in they order appear/in the order they appear/

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/qdev-properties.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Cornelia Huck]

On Mon, 10 Jul 2017 21:43:02 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Test case to detect the bug fixed by commit
> "qdev: fix the order compat and global properties are applied".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Cornelia Huck]

On Mon, 10 Jul 2017 21:43:03 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.
> 
> The bug addressed by that commit is now fixed in a better way by the
> commit "qdev: fix the order compat and global properties are applied".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/machine.c | 26 +++-----------------------
>  1 file changed, 3 insertions(+), 23 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Greg Kurz]

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

On Mon, 10 Jul 2017 21:43:02 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Test case to detect the bug fixed by commit
> "qdev: fix the order compat and global properties are applied".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

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

>  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 48e5b73..ef2951f 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -33,6 +33,8 @@
>  #define STATIC_TYPE(obj) \
>      OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
>  
> +#define TYPE_SUBCLASS "static_prop_subtype"
> +
>  #define PROP_DEFAULT 100
>  
>  typedef struct MyType {
> @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
>      .class_init = static_prop_class_init,
>  };
>  
> +static const TypeInfo subclass_type = {
> +    .name = TYPE_SUBCLASS,
> +    .parent = TYPE_STATIC_PROPS,
> +};
> +
>  /* Test simple static property setting to default value */
>  static void test_static_prop_subprocess(void)
>  {
> @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
>      g_test_trap_assert_stdout("");
>  }
>  
> +/* Test if global props affecting subclasses are applied in the right order */
> +static void test_subclass_global_props(void)
> +{
> +    MyType *mt;
> +    /* Global properties must be applied in the order they were registered */
> +    static GlobalProperty props[] = {
> +        { TYPE_STATIC_PROPS, "prop1", "101" },
> +        { TYPE_SUBCLASS,     "prop1", "102" },
> +        { TYPE_SUBCLASS,     "prop2", "103" },
> +        { TYPE_STATIC_PROPS, "prop2", "104" },
> +        {}
> +    };
> +
> +    qdev_prop_register_global_list(props);
> +
> +    mt = STATIC_TYPE(object_new(TYPE_SUBCLASS));
> +    qdev_init_nofail(DEVICE(mt));
> +
> +    g_assert_cmpuint(mt->prop1, ==, 102);
> +    g_assert_cmpuint(mt->prop2, ==, 104);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
>  
>      module_call_init(MODULE_INIT_QOM);
>      type_register_static(&static_prop_type);
> +    type_register_static(&subclass_type);
>      type_register_static(&dynamic_prop_type);
>      type_register_static(&hotplug_type);
>      type_register_static(&nohotplug_type);
> @@ -310,6 +340,9 @@ int main(int argc, char **argv)
>      g_test_add_func("/qdev/properties/dynamic/global/nouser",
>                      test_dynamic_globalprop_nouser);
>  
> +    g_test_add_func("/qdev/properties/global/subclass",
> +                    test_subclass_global_props);
> +
>      g_test_run();
>  
>      return 0;


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

Re: [Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Greg Kurz]

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

On Mon, 10 Jul 2017 21:43:03 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.
> 
> The bug addressed by that commit is now fixed in a better way by the
> commit "qdev: fix the order compat and global properties are applied".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

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

>  hw/core/machine.c | 26 +++-----------------------
>  1 file changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ecb5552..1d10b01 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -770,18 +770,11 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>      g_free(mc->name);
>  }
>  
> -static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
> -{
> -    GlobalProperty *p = opaque;
> -    register_compat_prop(object_class_get_name(oc), p->property, p->value);
> -}
> -
>  void machine_register_compat_props(MachineState *machine)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      int i;
>      GlobalProperty *p;
> -    ObjectClass *oc;
>  
>      if (!mc->compat_props) {
>          return;
> @@ -789,22 +782,9 @@ void machine_register_compat_props(MachineState *machine)
>  
>      for (i = 0; i < mc->compat_props->len; i++) {
>          p = g_array_index(mc->compat_props, GlobalProperty *, i);
> -        oc = object_class_by_name(p->driver);
> -        if (oc && object_class_is_abstract(oc)) {
> -            /* temporary hack to make sure we do not override
> -             * globals set explicitly on -global: if an abstract class
> -             * is on compat_props, register globals for all its
> -             * non-abstract subtypes instead.
> -             *
> -             * This doesn't solve the problem for cases where
> -             * a non-abstract typename mentioned on compat_props
> -             * has subclasses, like spapr-pci-host-bridge.
> -             */
> -            object_class_foreach(machine_register_compat_for_subclass,
> -                                 p->driver, false, p);
> -        } else {
> -            register_compat_prop(p->driver, p->property, p->value);
> -        }
> +        /* Machine compat_props must never cause errors: */
> +        p->errp = &error_abort;
> +        qdev_prop_register_global(p);
>      }
>  }
>  


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

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

[Halil Pasic]

On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> From: Greg Kurz <groug@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 <groug@kaod.org>
> Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
> Signed-off-by: Eduardo Habkost <ehabkost@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 <pasic@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).

Re: [Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Halil Pasic]

On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.
> 
> The bug addressed by that commit is now fixed in a better way by the
> commit "qdev: fix the order compat and global properties are applied".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>

Note: It is not like the effect of commit 0bcba41fe3 is canceled
out with your first patch in place. It depends on the client
code (the implementation of the individual devices) wether
this patch changes something or not. I did not check myself.
So the did you verify that nothing breaks with this change applies
here too.

> ---
>  hw/core/machine.c | 26 +++-----------------------
>  1 file changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ecb5552..1d10b01 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -770,18 +770,11 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>      g_free(mc->name);
>  }
> 
> -static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
> -{
> -    GlobalProperty *p = opaque;
> -    register_compat_prop(object_class_get_name(oc), p->property, p->value);
> -}
> -
>  void machine_register_compat_props(MachineState *machine)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      int i;
>      GlobalProperty *p;
> -    ObjectClass *oc;
> 
>      if (!mc->compat_props) {
>          return;
> @@ -789,22 +782,9 @@ void machine_register_compat_props(MachineState *machine)
> 
>      for (i = 0; i < mc->compat_props->len; i++) {
>          p = g_array_index(mc->compat_props, GlobalProperty *, i);
> -        oc = object_class_by_name(p->driver);
> -        if (oc && object_class_is_abstract(oc)) {
> -            /* temporary hack to make sure we do not override
> -             * globals set explicitly on -global: if an abstract class
> -             * is on compat_props, register globals for all its
> -             * non-abstract subtypes instead.
> -             *
> -             * This doesn't solve the problem for cases where
> -             * a non-abstract typename mentioned on compat_props
> -             * has subclasses, like spapr-pci-host-bridge.
> -             */
> -            object_class_foreach(machine_register_compat_for_subclass,
> -                                 p->driver, false, p);
> -        } else {
> -            register_compat_prop(p->driver, p->property, p->value);
> -        }
> +        /* Machine compat_props must never cause errors: */
> +        p->errp = &error_abort;
> +        qdev_prop_register_global(p);
>      }
>  }
> 

Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Halil Pasic]

On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> Test case to detect the bug fixed by commit
> "qdev: fix the order compat and global properties are applied".
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>

Please see below nevertheless.

> ---
>  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 48e5b73..ef2951f 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -33,6 +33,8 @@
>  #define STATIC_TYPE(obj) \
>      OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
> 
> +#define TYPE_SUBCLASS "static_prop_subtype"
> +
>  #define PROP_DEFAULT 100
> 
>  typedef struct MyType {
> @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
>      .class_init = static_prop_class_init,
>  };
> 
> +static const TypeInfo subclass_type = {
> +    .name = TYPE_SUBCLASS,
> +    .parent = TYPE_STATIC_PROPS,
> +};
> +
>  /* Test simple static property setting to default value */
>  static void test_static_prop_subprocess(void)
>  {
> @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
>      g_test_trap_assert_stdout("");
>  }
> 
> +/* Test if global props affecting subclasses are applied in the right order */

Now that I think about it this is affecting an  external and
(end-)user facing interface. You say this is the right order.
Based on what is this the right order? Do we need to update some
documentation too?

I've checked out the user manual. There we talk about 'driver' but
I could not find a spot where 'driver' is explained.

If I would have to translate between user manual terminology and
code terminology, I would say a driver is a not abstract class
inheriting from device. If that's right I wonder if it should
be allowed for a non-abstract class inheriting from device to
inherit form another non-abstract class.

Another question is -global with an abstract class seems to be
accepted on the command line. Is that an undocumented feature?

Sorry for expanding the front. I also trying to figure out the design
for my own benefit.

Regards,
Halil 


> +static void test_subclass_global_props(void)
> +{
> +    MyType *mt;
> +    /* Global properties must be applied in the order they were registered */
> +    static GlobalProperty props[] = {
> +        { TYPE_STATIC_PROPS, "prop1", "101" },
> +        { TYPE_SUBCLASS,     "prop1", "102" },
> +        { TYPE_SUBCLASS,     "prop2", "103" },
> +        { TYPE_STATIC_PROPS, "prop2", "104" },
> +        {}
> +    };
> +
> +    qdev_prop_register_global_list(props);
> +
> +    mt = STATIC_TYPE(object_new(TYPE_SUBCLASS));
> +    qdev_init_nofail(DEVICE(mt));
> +
> +    g_assert_cmpuint(mt->prop1, ==, 102);
> +    g_assert_cmpuint(mt->prop2, ==, 104);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> 
>      module_call_init(MODULE_INIT_QOM);
>      type_register_static(&static_prop_type);
> +    type_register_static(&subclass_type);
>      type_register_static(&dynamic_prop_type);
>      type_register_static(&hotplug_type);
>      type_register_static(&nohotplug_type);
> @@ -310,6 +340,9 @@ int main(int argc, char **argv)
>      g_test_add_func("/qdev/properties/dynamic/global/nouser",
>                      test_dynamic_globalprop_nouser);
> 
> +    g_test_add_func("/qdev/properties/global/subclass",
> +                    test_subclass_global_props);
> +
>      g_test_run();
> 
>      return 0;
> 

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

[Eduardo Habkost]

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 <groug@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 <groug@kaod.org>
> > Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@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 <pasic@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.

The main sources of global properties we have are:
* MachineClass::compat_props
* -global command-line option
* -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.
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.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg416670.html
    https://www.mail-archive.com/qemu-devel@nongnu.org/msg416985.html

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Eduardo Habkost]

On Wed, Jul 12, 2017 at 08:06:14PM +0200, Halil Pasic wrote:
> 
> 
> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> > Test case to detect the bug fixed by commit
> > "qdev: fix the order compat and global properties are applied".
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> Please see below nevertheless.
> 
> > ---
> >  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> > index 48e5b73..ef2951f 100644
> > --- a/tests/test-qdev-global-props.c
> > +++ b/tests/test-qdev-global-props.c
> > @@ -33,6 +33,8 @@
> >  #define STATIC_TYPE(obj) \
> >      OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
> > 
> > +#define TYPE_SUBCLASS "static_prop_subtype"
> > +
> >  #define PROP_DEFAULT 100
> > 
> >  typedef struct MyType {
> > @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
> >      .class_init = static_prop_class_init,
> >  };
> > 
> > +static const TypeInfo subclass_type = {
> > +    .name = TYPE_SUBCLASS,
> > +    .parent = TYPE_STATIC_PROPS,
> > +};
> > +
> >  /* Test simple static property setting to default value */
> >  static void test_static_prop_subprocess(void)
> >  {
> > @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
> >      g_test_trap_assert_stdout("");
> >  }
> > 
> > +/* Test if global props affecting subclasses are applied in the right order */
> 
> Now that I think about it this is affecting an  external and
> (end-)user facing interface. You say this is the right order.
> Based on what is this the right order? Do we need to update some
> documentation too?

-global documentation is already very poor, as you have noticed:

> 
> I've checked out the user manual. There we talk about 'driver' but
> I could not find a spot where 'driver' is explained.

Yes.  For that reason, it looks like there's nothing to be
updated on the existing (poor) documentation.  I will re-read it
to be sure.

> 
> If I would have to translate between user manual terminology and
> code terminology, I would say a driver is a not abstract class
> inheriting from device. If that's right I wonder if it should
> be allowed for a non-abstract class inheriting from device to
> inherit form another non-abstract class.

We could, but this wouldn't save us from having to decide what to
do if people are already using those non-abstract superclasses
with -global.  e.g.: "-global spapr-pci-host-bridge.FOO=BAR"
(spapr-pci-host-bridge is the parent of spapr-pci-vfio-host-bridge).

This means a new rule like this would necessarily break
command-line compatibility.  Probably not a blocker in the
spapr-pci-host-bridge case, but to me it looks like the rule
would cause more problems than it would solve.

> 
> Another question is -global with an abstract class seems to be
> accepted on the command line. Is that an undocumented feature?

Considering that we never documented that "driver" could be a
superclass, that's true: it is an undocumented feature.

However, it is a feature people are likely to be relying upon, to
configure a feature on all devices of a given type.


> 
> Sorry for expanding the front. I also trying to figure out the design
> for my own benefit.

Your comments and suggestions are very welcome, thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Eduardo Habkost]

On Wed, Jul 12, 2017 at 07:49:22PM +0200, Halil Pasic wrote:
> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> > This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.
> > 
> > The bug addressed by that commit is now fixed in a better way by the
> > commit "qdev: fix the order compat and global properties are applied".
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> Note: It is not like the effect of commit 0bcba41fe3 is canceled
> out with your first patch in place. It depends on the client
> code (the implementation of the individual devices) wether
> this patch changes something or not. I did not check myself.
> So the did you verify that nothing breaks with this change applies
> here too.

I don't get this part.  I don't see how individual devices
implementation will be able to affect the outcome after this
patch is applied.

GlobalProperty::driver is not used as input for
object_property_parse() at all (see qdev_prop_set_globals()).
This means exactly the same property setter is invoked when
registering "<superclass>.<property>" or "<subclass>.<property>".
The only difference introduced by this series is in the ordering
of the object_property_parse() calls.

And even the object_property_parse() call ordering is not
affected by this patch at all, because of patch 1/3.  Patch 1/3
will ensure the properties will be applied in exactly the same
order they were registered, so this patch should introduce
absolutely no behavior change on any device.

-- 
Eduardo

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 <groug@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 <groug@kaod.org>
>>> Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@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 <pasic@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
> 

Re: [Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Halil Pasic]

On 07/12/2017 09:20 PM, Eduardo Habkost wrote:
> On Wed, Jul 12, 2017 at 07:49:22PM +0200, Halil Pasic wrote:
>> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
>>> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.
>>>
>>> The bug addressed by that commit is now fixed in a better way by the
>>> commit "qdev: fix the order compat and global properties are applied".
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>
>> Note: It is not like the effect of commit 0bcba41fe3 is canceled
>> out with your first patch in place. It depends on the client
>> code (the implementation of the individual devices) wether
>> this patch changes something or not. I did not check myself.
>> So the did you verify that nothing breaks with this change applies
>> here too.
> 
> I don't get this part.  I don't see how individual devices
> implementation will be able to affect the outcome after this
> patch is applied.
> 
> GlobalProperty::driver is not used as input for
> object_property_parse() at all (see qdev_prop_set_globals()).
> This means exactly the same property setter is invoked when
> registering "<superclass>.<property>" or "<subclass>.<property>".
> The only difference introduced by this series is in the ordering
> of the object_property_parse() calls.
> 
> And even the object_property_parse() call ordering is not
> affected by this patch at all, because of patch 1/3.  Patch 1/3
> will ensure the properties will be applied in exactly the same
> order they were registered, so this patch should introduce
> absolutely no behavior change on any device.
> 

Resolving this disagreement IMHO depends on resolving our disagreement
about patch 1. Let us postpone this until we have an agreement
there.

Cheers,
Halil

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

[Eduardo Habkost]

On Thu, Jul 13, 2017 at 01:54:01PM +0200, Halil Pasic wrote:
> 
> 
> 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 <groug@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 <groug@kaod.org>
> >>> Message-Id: <148103887228.22326.478406873609299999.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
> >>> Signed-off-by: Eduardo Habkost <ehabkost@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 <pasic@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.

Good.  :)

> 
> > 
> > The main sources of global properties we have are:
> > * MachineClass::compat_props> * -global command-line option
> 
> I was thinking about these two.

Good, this is what we're really trying to address, so let's
review that:

> 
> > * -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.

Good catch!  You are correct, and your experiment below is
correct.  But I thought no non-abstract superclasses where used
on compat_props on any machine-type (then this patch wouldn't
have any visible effect in the current tree).

However, commit 0bcba41f has this note, which I had forgot about:

    Note that there's one case that won't be fixed by this hack:
    "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be
    able to override compat_props, because spapr-pci-host-bridge is
    not an abstract class.

We really have a
spapr-pci-host-bridge.dynamic-reconfiguration=off entry in
compat_props for pseries-2.3.

This means this series will also fix the ordering between
compat_props and -global if "-global
spapr-pci-vfio-host-bridge.dynamic-reconfiguration=..." is used
with machine-type pseries <= 2.3.


> 
> 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.

I think pseries and spapr-pci-vfio-host-bridge is an existing
example that doesn't require changing the source.

> 
> > 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.

So, correcting myself: the only behavior change regarding
compat_props introduced by this patch should be when "-global
spapr-pci-vfio-host-bridge.dynamic-reconfiguration=on" is used
with machine-type pseries <= 2.3.  Now -global will correctly
override the entry in compat_props.

I didn't confirm if we have other non-abstract superclasses in
compat_props added to qemu.git after commit 0bcba41f, though.


> > 
> > 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
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] Revert "machine: Convert abstract typename on compat_props to subclass names"

[Eduardo Habkost]

On Thu, Jul 13, 2017 at 02:11:09PM +0200, Halil Pasic wrote:
> 
> 
> On 07/12/2017 09:20 PM, Eduardo Habkost wrote:
> > On Wed, Jul 12, 2017 at 07:49:22PM +0200, Halil Pasic wrote:
> >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> >>> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2.
> >>>
> >>> The bug addressed by that commit is now fixed in a better way by the
> >>> commit "qdev: fix the order compat and global properties are applied".
> >>>
> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>
> >> Note: It is not like the effect of commit 0bcba41fe3 is canceled
> >> out with your first patch in place. It depends on the client
> >> code (the implementation of the individual devices) wether
> >> this patch changes something or not. I did not check myself.
> >> So the did you verify that nothing breaks with this change applies
> >> here too.
> > 
> > I don't get this part.  I don't see how individual devices
> > implementation will be able to affect the outcome after this
> > patch is applied.
> > 
> > GlobalProperty::driver is not used as input for
> > object_property_parse() at all (see qdev_prop_set_globals()).
> > This means exactly the same property setter is invoked when
> > registering "<superclass>.<property>" or "<subclass>.<property>".
> > The only difference introduced by this series is in the ordering
> > of the object_property_parse() calls.
> > 
> > And even the object_property_parse() call ordering is not
> > affected by this patch at all, because of patch 1/3.  Patch 1/3
> > will ensure the properties will be applied in exactly the same
> > order they were registered, so this patch should introduce
> > absolutely no behavior change on any device.
> > 
> 
> Resolving this disagreement IMHO depends on resolving our disagreement
> about patch 1. Let us postpone this until we have an agreement
> there.

Your observation on patch 1 was correct, so the ordering of
object_property_parse() calls will change if using
pseries <= 2.3 and "-global spapr-pci-vfio-host-bridge".

-- 
Eduardo

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

[Halil Pasic]

On 07/13/2017 06:15 PM, Eduardo Habkost wrote:
> On Thu, Jul 13, 2017 at 01:54:01PM +0200, Halil Pasic wrote:
>>
>>
>> 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 <groug@kaod.org>
[..]
>>>> 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 <pasic@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.
> 
> Good.  :)
> 
>>
>>>
>>> The main sources of global properties we have are:
>>> * MachineClass::compat_props> * -global command-line option
>>
>> I was thinking about these two.
> 
> Good, this is what we're really trying to address, so let's
> review that:
> 
>>
>>> * -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.
> 
> Good catch!  You are correct, and your experiment below is
> correct.  But I thought no non-abstract superclasses where used
> on compat_props on any machine-type (then this patch wouldn't
> have any visible effect in the current tree).
> 
> However, commit 0bcba41f has this note, which I had forgot about:
> 
>     Note that there's one case that won't be fixed by this hack:
>     "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be
>     able to override compat_props, because spapr-pci-host-bridge is
>     not an abstract class.
> 
> We really have a
> spapr-pci-host-bridge.dynamic-reconfiguration=off entry in
> compat_props for pseries-2.3.
> 
> This means this series will also fix the ordering between
> compat_props and -global if "-global
> spapr-pci-vfio-host-bridge.dynamic-reconfiguration=..." is used
> with machine-type pseries <= 2.3.
> 
> 
>>
>> 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.
> 
> I think pseries and spapr-pci-vfio-host-bridge is an existing
> example that doesn't require changing the source.
> 
>>
>>> 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.
> 
> So, correcting myself: the only behavior change regarding
> compat_props introduced by this patch should be when "-global
> spapr-pci-vfio-host-bridge.dynamic-reconfiguration=on" is used
> with machine-type pseries <= 2.3.  Now -global will correctly
> override the entry in compat_props.
> 
> I didn't confirm if we have other non-abstract superclasses in
> compat_props added to qemu.git after commit 0bcba41f, though.
> 
>

I don't understand the 'added after commit 0bcba41' as this commit
affects only abstract superclasses. But it does not really matter
for me (see below).

To sum it up form my perspective:
1) My r-b still stands for the same reasons I've stated at the
beginning.
2) The changes for sure affect an external interface (command
line) and IMHO also an internal API. Both changes are for me
without a doubt for the better. 
3) For the external one, updating the documentation would in my opinion
really be a good idea. (We discussed this in the other thread).
4) Regarding the internal API change, there are two ways of thinking
about it: in the abstract forgetting about the "internal" from 
"internal API", and by looking at the interaction with all the client
code. My little analysis was restricted to the first, because I wanted
to understand the changes, and wanted to be sure we understand the
changes. I wanted to avoid doing the second part too. I  do think we
just have to make sure no client code is adversely affected by it. Now 
I've double checked, the stuff supported on s390x is not (adversely)
affected. For the non s390x stuff I will just take your word that
nobody is adversely affected.
5) I think a sentence or two in the commit message about why is the
external change OK, and why doesn't the internal API change require
any client code changes would not have hurt.

In general, I think we are in agreement.

Halil
 
>>>
>>> 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
>>>
>>
> 

Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Halil Pasic]

On 07/12/2017 08:48 PM, Eduardo Habkost wrote:
> On Wed, Jul 12, 2017 at 08:06:14PM +0200, Halil Pasic wrote:
>>
>>
>> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
>>> Test case to detect the bug fixed by commit
>>> "qdev: fix the order compat and global properties are applied".
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>
>> Please see below nevertheless.
>>
>>> ---
>>>  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
>>> index 48e5b73..ef2951f 100644
>>> --- a/tests/test-qdev-global-props.c
>>> +++ b/tests/test-qdev-global-props.c
>>> @@ -33,6 +33,8 @@
>>>  #define STATIC_TYPE(obj) \
>>>      OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
>>>
>>> +#define TYPE_SUBCLASS "static_prop_subtype"
>>> +
>>>  #define PROP_DEFAULT 100
>>>
>>>  typedef struct MyType {
>>> @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
>>>      .class_init = static_prop_class_init,
>>>  };
>>>
>>> +static const TypeInfo subclass_type = {
>>> +    .name = TYPE_SUBCLASS,
>>> +    .parent = TYPE_STATIC_PROPS,
>>> +};
>>> +
>>>  /* Test simple static property setting to default value */
>>>  static void test_static_prop_subprocess(void)
>>>  {
>>> @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
>>>      g_test_trap_assert_stdout("");
>>>  }
>>>
>>> +/* Test if global props affecting subclasses are applied in the right order */
>>
>> Now that I think about it this is affecting an  external and
>> (end-)user facing interface. You say this is the right order.
>> Based on what is this the right order? Do we need to update some
>> documentation too?
> 
> -global documentation is already very poor, as you have noticed:
> 
>>
>> I've checked out the user manual. There we talk about 'driver' but
>> I could not find a spot where 'driver' is explained.
> 
> Yes.  For that reason, it looks like there's nothing to be
> updated on the existing (poor) documentation.  I will re-read it
> to be sure.
> 

I don't agree. IMHO poor is reason enough to update.

>>
>> If I would have to translate between user manual terminology and
>> code terminology, I would say a driver is a not abstract class
>> inheriting from device. If that's right I wonder if it should
>> be allowed for a non-abstract class inheriting from device to
>> inherit form another non-abstract class.
> 
> We could, but this wouldn't save us from having to decide what to
> do if people are already using those non-abstract superclasses
> with -global.  e.g.: "-global spapr-pci-host-bridge.FOO=BAR"
> (spapr-pci-host-bridge is the parent of spapr-pci-vfio-host-bridge).
> 
> This means a new rule like this would necessarily break
> command-line compatibility.  Probably not a blocker in the
> spapr-pci-host-bridge case, but to me it looks like the rule
> would cause more problems than it would solve.

We are already breaking the command line compatibility with this
series aren't we?

You may be right about the cause more problems than it solves though.
I may end up thinking some more about this (or forgetting about it).
> 
>>
>> Another question is -global with an abstract class seems to be
>> accepted on the command line. Is that an undocumented feature?
> 
> Considering that we never documented that "driver" could be a
> superclass, that's true: it is an undocumented feature.
> 
> However, it is a feature people are likely to be relying upon, to
> configure a feature on all devices of a given type.
> 

Nod. I do however see a difference between abstract and non-abstract.

> 
>>
>> Sorry for expanding the front. I also trying to figure out the design
>> for my own benefit.
> 
> Your comments and suggestions are very welcome, thanks!
> 

Thanks!

Regards,
Halil

Re: [Qemu-devel] [PATCH 2/3] test-qdev-global-props: Test global property ordering

[Eduardo Habkost]

On Sun, Jul 16, 2017 at 02:35:49PM +0200, Halil Pasic wrote:
> On 07/12/2017 08:48 PM, Eduardo Habkost wrote:
> > On Wed, Jul 12, 2017 at 08:06:14PM +0200, Halil Pasic wrote:
> >>
> >>
> >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> >>> Test case to detect the bug fixed by commit
> >>> "qdev: fix the order compat and global properties are applied".
> >>>
> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>
> >> Please see below nevertheless.
> >>
> >>> ---
> >>>  tests/test-qdev-global-props.c | 33 +++++++++++++++++++++++++++++++++
> >>>  1 file changed, 33 insertions(+)
> >>>
> >>> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> >>> index 48e5b73..ef2951f 100644
> >>> --- a/tests/test-qdev-global-props.c
> >>> +++ b/tests/test-qdev-global-props.c
> >>> @@ -33,6 +33,8 @@
> >>>  #define STATIC_TYPE(obj) \
> >>>      OBJECT_CHECK(MyType, (obj), TYPE_STATIC_PROPS)
> >>>
> >>> +#define TYPE_SUBCLASS "static_prop_subtype"
> >>> +
> >>>  #define PROP_DEFAULT 100
> >>>
> >>>  typedef struct MyType {
> >>> @@ -63,6 +65,11 @@ static const TypeInfo static_prop_type = {
> >>>      .class_init = static_prop_class_init,
> >>>  };
> >>>
> >>> +static const TypeInfo subclass_type = {
> >>> +    .name = TYPE_SUBCLASS,
> >>> +    .parent = TYPE_STATIC_PROPS,
> >>> +};
> >>> +
> >>>  /* Test simple static property setting to default value */
> >>>  static void test_static_prop_subprocess(void)
> >>>  {
> >>> @@ -279,12 +286,35 @@ static void test_dynamic_globalprop_nouser(void)
> >>>      g_test_trap_assert_stdout("");
> >>>  }
> >>>
> >>> +/* Test if global props affecting subclasses are applied in the right order */
> >>
> >> Now that I think about it this is affecting an  external and
> >> (end-)user facing interface. You say this is the right order.
> >> Based on what is this the right order? Do we need to update some
> >> documentation too?
> > 
> > -global documentation is already very poor, as you have noticed:
> > 
> >>
> >> I've checked out the user manual. There we talk about 'driver' but
> >> I could not find a spot where 'driver' is explained.
> > 
> > Yes.  For that reason, it looks like there's nothing to be
> > updated on the existing (poor) documentation.  I will re-read it
> > to be sure.
> > 
> 
> I don't agree. IMHO poor is reason enough to update.

I agree we should update it.  I just meant I don't see any
existing documentation that will become incorrect/outdated when
we apply this patch.

> 
> >>
> >> If I would have to translate between user manual terminology and
> >> code terminology, I would say a driver is a not abstract class
> >> inheriting from device. If that's right I wonder if it should
> >> be allowed for a non-abstract class inheriting from device to
> >> inherit form another non-abstract class.
> > 
> > We could, but this wouldn't save us from having to decide what to
> > do if people are already using those non-abstract superclasses
> > with -global.  e.g.: "-global spapr-pci-host-bridge.FOO=BAR"
> > (spapr-pci-host-bridge is the parent of spapr-pci-vfio-host-bridge).
> > 
> > This means a new rule like this would necessarily break
> > command-line compatibility.  Probably not a blocker in the
> > spapr-pci-host-bridge case, but to me it looks like the rule
> > would cause more problems than it would solve.
> 
> We are already breaking the command line compatibility with this
> series aren't we?
> 
> You may be right about the cause more problems than it solves though.
> I may end up thinking some more about this (or forgetting about it).

Yeah.  Breaking compatibility is not a blocker, but one
additional obstacle for changing the rules.

> > 
> >>
> >> Another question is -global with an abstract class seems to be
> >> accepted on the command line. Is that an undocumented feature?
> > 
> > Considering that we never documented that "driver" could be a
> > superclass, that's true: it is an undocumented feature.
> > 
> > However, it is a feature people are likely to be relying upon, to
> > configure a feature on all devices of a given type.
> > 
> 
> Nod. I do however see a difference between abstract and non-abstract.

I think I see your point.  The semantics of "-global" would be
clearer if all non-leaf classes were abstract.

-- 
Eduardo