Let's make compatiblity properties an interface, so that objects other
than QDev can benefit from having machine compatiblity properties.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/boards.h    |  2 ++
 hw/core/compat-props.c | 55 ++++++++++++++++++++++++++++++++++++++++++
 hw/core/qdev.c         | 24 +++---------------
 MAINTAINERS            |  1 +
 hw/core/Makefile.objs  |  1 +
 tests/Makefile.include |  1 +
 6 files changed, 64 insertions(+), 20 deletions(-)
 create mode 100644 hw/core/compat-props.c

diff --git a/include/hw/boards.h b/include/hw/boards.h
index f743d9d4a4..77d1fc1bef 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -9,6 +9,8 @@
 #include "qom/object.h"
 #include "qom/cpu.h"
 
+#define TYPE_COMPAT_PROPS "compat-props"
+
 /**
  * memory_region_allocate_system_memory - Allocate a board's main memory
  * @mr: the #MemoryRegion to be initialized
diff --git a/hw/core/compat-props.c b/hw/core/compat-props.c
new file mode 100644
index 0000000000..538378e71f
--- /dev/null
+++ b/hw/core/compat-props.c
@@ -0,0 +1,55 @@
+/*
+ * QEMU Machine compat properties
+ *
+ * Copyright (C) 2018 Red Hat Inc
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/boards.h"
+#include "qapi/error.h"
+
+typedef struct CompatProps CompatProps;
+
+typedef struct CompatPropsClass {
+    InterfaceClass parent_class;
+} CompatPropsClass;
+
+static const GPtrArray *ac_compat_props;
+static const GPtrArray *mc_compat_props;
+
+void accel_register_compat_props(const GPtrArray *props)
+{
+    ac_compat_props = props;
+}
+
+void machine_register_compat_props(const GPtrArray *props)
+{
+    mc_compat_props = props;
+}
+
+static void compat_props_post_init(Object *obj)
+{
+    if (ac_compat_props) {
+        object_apply_global_props(obj, ac_compat_props, &error_abort);
+    }
+    if (mc_compat_props) {
+        object_apply_global_props(obj, mc_compat_props, &error_abort);
+    }
+}
+
+static void compat_props_register_types(void)
+{
+    static const TypeInfo cp_interface_info = {
+        .name          = TYPE_COMPAT_PROPS,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(CompatPropsClass),
+        .instance_post_init = compat_props_post_init,
+    };
+
+    type_register_static(&cp_interface_info);
+}
+
+type_init(compat_props_register_types)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 3b31b2c025..b0ee05f837 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -970,28 +970,8 @@ static void device_initfn(Object *obj)
     QLIST_INIT(&dev->gpios);
 }
 
-static const GPtrArray *ac_compat_props;
-static const GPtrArray *mc_compat_props;
-
-void accel_register_compat_props(const GPtrArray *props)
-{
-    ac_compat_props = props;
-}
-
-void machine_register_compat_props(const GPtrArray *props)
-{
-    mc_compat_props = props;
-}
-
 static void device_post_init(Object *obj)
 {
-    if (ac_compat_props) {
-        object_apply_global_props(obj, ac_compat_props, &error_abort);
-    }
-    if (mc_compat_props) {
-        object_apply_global_props(obj, mc_compat_props, &error_abort);
-    }
-
     qdev_prop_set_globals(DEVICE(obj));
 }
 
@@ -1124,6 +1104,10 @@ static const TypeInfo device_type_info = {
     .class_init = device_class_init,
     .abstract = true,
     .class_size = sizeof(DeviceClass),
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_COMPAT_PROPS },
+        { }
+    }
 };
 
 static void qdev_register_types(void)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9410bbb7cf..adff09627f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1145,6 +1145,7 @@ Machine core
 M: Eduardo Habkost <ehabkost@redhat.com>
 M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
 S: Supported
+F: hw/core/compat-props.c
 F: hw/core/machine.c
 F: hw/core/null-machine.c
 F: include/hw/boards.h
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index a799c83815..f15b3c970a 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,5 +1,6 @@
 # core qdev-related obj files, also used by *-user:
 common-obj-y += qdev.o qdev-properties.o
+common-obj-y += compat-props.o
 common-obj-y += bus.o reset.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
 common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
diff --git a/tests/Makefile.include b/tests/Makefile.include
index fb0b449c02..fc74358c0a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -567,6 +567,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
 	hw/core/irq.o \
 	hw/core/fw-path-provider.o \
 	hw/core/reset.o \
+	hw/core/compat-props.o \
 	$(test-qapi-obj-y)
 tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
 	migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \
-- 
2.20.0.rc1

On Tue, Nov 27, 2018 at 01:27:58PM +0400, Marc-André Lureau wrote:
> Let's make compatiblity properties an interface, so that objects other
> than QDev can benefit from having machine compatiblity properties.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[...]
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 3b31b2c025..b0ee05f837 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -970,28 +970,8 @@ static void device_initfn(Object *obj)
>      QLIST_INIT(&dev->gpios);
>  }
>  
> -static const GPtrArray *ac_compat_props;
> -static const GPtrArray *mc_compat_props;
> -
> -void accel_register_compat_props(const GPtrArray *props)
> -{
> -    ac_compat_props = props;
> -}
> -
> -void machine_register_compat_props(const GPtrArray *props)
> -{
> -    mc_compat_props = props;
> -}
> -
>  static void device_post_init(Object *obj)
>  {
> -    if (ac_compat_props) {
> -        object_apply_global_props(obj, ac_compat_props, &error_abort);
> -    }
> -    if (mc_compat_props) {
> -        object_apply_global_props(obj, mc_compat_props, &error_abort);
> -    }
> -
>      qdev_prop_set_globals(DEVICE(obj));
>  }
>  
> @@ -1124,6 +1104,10 @@ static const TypeInfo device_type_info = {
>      .class_init = device_class_init,
>      .abstract = true,
>      .class_size = sizeof(DeviceClass),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_COMPAT_PROPS },
> +        { }
> +    }

At first I thought TYPE_COMPAT_PROPS was a practical way to
implement this feature, but now I'm worried: the ordering between
compat_props_post_init() qdev_prop_set_globals() is very
important (user-provided globals must always be set after compat
props), and here the ordering is implicit and easy to break
accidentally.

What if instead of a QOM interface we just provide a simple
object_apply_compat_props() function?  e.g.:

qdev.c:

  static void device_post_init(Object *obj)
  {
      object_apply_compat_props(obj);
      apply_user_provided_globals(obj);
  }

object_interface.c:

  void user_creatable_complete(Object *obj, Error **errp)
  {
      object_apply_compat_props(obj);
      ...
      ucc->complete(...)
  }

Most people don't understand QOM interfaces and their
initialization ordering rules.  Everybody understands C function
calls.

> [...]

-- 
Eduardo

On Thu, 29 Nov 2018 15:49:00 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Nov 27, 2018 at 01:27:58PM +0400, Marc-André Lureau wrote:
> > Let's make compatiblity properties an interface, so that objects other
> > than QDev can benefit from having machine compatiblity properties.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
> [...]
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 3b31b2c025..b0ee05f837 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -970,28 +970,8 @@ static void device_initfn(Object *obj)
> >      QLIST_INIT(&dev->gpios);
> >  }
> >  
> > -static const GPtrArray *ac_compat_props;
> > -static const GPtrArray *mc_compat_props;
> > -
> > -void accel_register_compat_props(const GPtrArray *props)
> > -{
> > -    ac_compat_props = props;
> > -}
> > -
> > -void machine_register_compat_props(const GPtrArray *props)
> > -{
> > -    mc_compat_props = props;
> > -}
> > -
> >  static void device_post_init(Object *obj)
> >  {
> > -    if (ac_compat_props) {
> > -        object_apply_global_props(obj, ac_compat_props, &error_abort);
> > -    }
> > -    if (mc_compat_props) {
> > -        object_apply_global_props(obj, mc_compat_props, &error_abort);
> > -    }
> > -
> >      qdev_prop_set_globals(DEVICE(obj));
> >  }
> >  
> > @@ -1124,6 +1104,10 @@ static const TypeInfo device_type_info = {
> >      .class_init = device_class_init,
> >      .abstract = true,
> >      .class_size = sizeof(DeviceClass),
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_COMPAT_PROPS },
> > +        { }
> > +    }  
> 
> At first I thought TYPE_COMPAT_PROPS was a practical way to
> implement this feature, but now I'm worried: the ordering between
> compat_props_post_init() qdev_prop_set_globals() is very
> important (user-provided globals must always be set after compat
> props), and here the ordering is implicit and easy to break
> accidentally.
> 
> What if instead of a QOM interface we just provide a simple
> object_apply_compat_props() function?  e.g.:
> 
> qdev.c:
> 
>   static void device_post_init(Object *obj)
>   {
>       object_apply_compat_props(obj);
>       apply_user_provided_globals(obj);
>   }
> 
> object_interface.c:
> 
>   void user_creatable_complete(Object *obj, Error **errp)
>   {
>       object_apply_compat_props(obj);
>       ...
>       ucc->complete(...)
>   }
this would work as long as nothing is happening between
object_new() ... user_creatable_complete() but look at
current users of user_creatable_complete() so it's
fragile too.

Reasons for compat props interface are the same as for
instance_post_init/device_post_init.

the thing we can do here is getting rid of device_post_init
and making device override TYPE_COMPAT_PROPS::instance_post_init
to make explicit ordering like we do everywhere else:

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6b3cc55..46ad6f5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1033,11 +1033,19 @@ static void device_unparent(Object *obj)
     }
 }
 
+device_compat_props()
+{
+    dc->parent_compat_props()
+    apply_global_props()
+}
+
 static void device_class_init(ObjectClass *class, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(class);
 
     class->unparent = device_unparent;
+    dc->parent_compat_props = COMPAT_PROPS_GET_CLASS(class)->instance_post_init
+    COMPAT_PROPS_GET_CLASS(class)->instance_post_init = device_compat_props()
 
     /* by default all devices were considered as hotpluggable,
      * so with intent to check it in generic qdev_unplug() /

> Most people don't understand QOM interfaces and their
> initialization ordering rules.  Everybody understands C function
> calls.
> 
> > [...]  
> 

On Fri, Nov 30, 2018 at 01:34:56PM +0100, Igor Mammedov wrote:
> On Thu, 29 Nov 2018 15:49:00 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Nov 27, 2018 at 01:27:58PM +0400, Marc-André Lureau wrote:
> > > Let's make compatiblity properties an interface, so that objects other
> > > than QDev can benefit from having machine compatiblity properties.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
> > [...]
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 3b31b2c025..b0ee05f837 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -970,28 +970,8 @@ static void device_initfn(Object *obj)
> > >      QLIST_INIT(&dev->gpios);
> > >  }
> > >  
> > > -static const GPtrArray *ac_compat_props;
> > > -static const GPtrArray *mc_compat_props;
> > > -
> > > -void accel_register_compat_props(const GPtrArray *props)
> > > -{
> > > -    ac_compat_props = props;
> > > -}
> > > -
> > > -void machine_register_compat_props(const GPtrArray *props)
> > > -{
> > > -    mc_compat_props = props;
> > > -}
> > > -
> > >  static void device_post_init(Object *obj)
> > >  {
> > > -    if (ac_compat_props) {
> > > -        object_apply_global_props(obj, ac_compat_props, &error_abort);
> > > -    }
> > > -    if (mc_compat_props) {
> > > -        object_apply_global_props(obj, mc_compat_props, &error_abort);
> > > -    }
> > > -
> > >      qdev_prop_set_globals(DEVICE(obj));
> > >  }
> > >  
> > > @@ -1124,6 +1104,10 @@ static const TypeInfo device_type_info = {
> > >      .class_init = device_class_init,
> > >      .abstract = true,
> > >      .class_size = sizeof(DeviceClass),
> > > +    .interfaces = (InterfaceInfo[]) {
> > > +        { TYPE_COMPAT_PROPS },
> > > +        { }
> > > +    }  
> > 
> > At first I thought TYPE_COMPAT_PROPS was a practical way to
> > implement this feature, but now I'm worried: the ordering between
> > compat_props_post_init() qdev_prop_set_globals() is very
> > important (user-provided globals must always be set after compat
> > props), and here the ordering is implicit and easy to break
> > accidentally.
> > 
> > What if instead of a QOM interface we just provide a simple
> > object_apply_compat_props() function?  e.g.:
> > 
> > qdev.c:
> > 
> >   static void device_post_init(Object *obj)
> >   {
> >       object_apply_compat_props(obj);
> >       apply_user_provided_globals(obj);
> >   }
> > 
> > object_interface.c:
> > 
> >   void user_creatable_complete(Object *obj, Error **errp)
> >   {
> >       object_apply_compat_props(obj);
> >       ...
> >       ucc->complete(...)
> >   }
> this would work as long as nothing is happening between
> object_new() ... user_creatable_complete() but look at
> current users of user_creatable_complete() so it's
> fragile too.

You are right.  This could be solved by:

  void user_creatable_post_init(Object *obj)
  {
      object_apply_compat_props(obj);
  }


> 
> Reasons for compat props interface are the same as for
> instance_post_init/device_post_init.
> 
> the thing we can do here is getting rid of device_post_init
> and making device override TYPE_COMPAT_PROPS::instance_post_init
> to make explicit ordering like we do everywhere else:
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55..46ad6f5 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1033,11 +1033,19 @@ static void device_unparent(Object *obj)
>      }
>  }
>  
> +device_compat_props()
> +{
> +    dc->parent_compat_props()
> +    apply_global_props()
> +}
> +
>  static void device_class_init(ObjectClass *class, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(class);
>  
>      class->unparent = device_unparent;
> +    dc->parent_compat_props = COMPAT_PROPS_GET_CLASS(class)->instance_post_init
> +    COMPAT_PROPS_GET_CLASS(class)->instance_post_init = device_compat_props()
>  
>      /* by default all devices were considered as hotpluggable,
>       * so with intent to check it in generic qdev_unplug() /

The only advantage I saw in TYPE_COMPAT_PROPS interface was to
easily allow objects to implement behavior without manually
implementing post_init.  Now we can't use the interface without
an even more complex way of overriding post_init, so what's the
point?

Why not just call object_apply_compat_props() at
device_post_init()?

> 
> > Most people don't understand QOM interfaces and their
> > initialization ordering rules.  Everybody understands C function
> > calls.
> > 
> > > [...]  
> > 
> 

-- 
Eduardo

On Tue, 27 Nov 2018 13:27:58 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> Let's make compatiblity properties an interface, so that objects other
> than QDev can benefit from having machine compatiblity properties.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/boards.h    |  2 ++
>  hw/core/compat-props.c | 55 ++++++++++++++++++++++++++++++++++++++++++
>  hw/core/qdev.c         | 24 +++---------------
>  MAINTAINERS            |  1 +
>  hw/core/Makefile.objs  |  1 +
>  tests/Makefile.include |  1 +
>  6 files changed, 64 insertions(+), 20 deletions(-)
>  create mode 100644 hw/core/compat-props.c
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index f743d9d4a4..77d1fc1bef 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -9,6 +9,8 @@
>  #include "qom/object.h"
>  #include "qom/cpu.h"
>  
> +#define TYPE_COMPAT_PROPS "compat-props"
> +
>  /**
>   * memory_region_allocate_system_memory - Allocate a board's main memory
>   * @mr: the #MemoryRegion to be initialized
> diff --git a/hw/core/compat-props.c b/hw/core/compat-props.c
> new file mode 100644
> index 0000000000..538378e71f
> --- /dev/null
> +++ b/hw/core/compat-props.c
> @@ -0,0 +1,55 @@
> +/*
> + * QEMU Machine compat properties
> + *
> + * Copyright (C) 2018 Red Hat Inc
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/boards.h"
> +#include "qapi/error.h"
> +
> +typedef struct CompatProps CompatProps;
> +
> +typedef struct CompatPropsClass {
> +    InterfaceClass parent_class;
> +} CompatPropsClass;
> +
> +static const GPtrArray *ac_compat_props;
> +static const GPtrArray *mc_compat_props;
> +
> +void accel_register_compat_props(const GPtrArray *props)
> +{
> +    ac_compat_props = props;
> +}
> +
> +void machine_register_compat_props(const GPtrArray *props)
> +{
> +    mc_compat_props = props;
> +}
> +
> +static void compat_props_post_init(Object *obj)
> +{
> +    if (ac_compat_props) {
> +        object_apply_global_props(obj, ac_compat_props, &error_abort);
> +    }
> +    if (mc_compat_props) {
> +        object_apply_global_props(obj, mc_compat_props, &error_abort);
> +    }
> +}
> +
> +static void compat_props_register_types(void)
> +{
> +    static const TypeInfo cp_interface_info = {
> +        .name          = TYPE_COMPAT_PROPS,
> +        .parent        = TYPE_INTERFACE,
> +        .class_size = sizeof(CompatPropsClass),
> +        .instance_post_init = compat_props_post_init,
you went here for implict way to set compat props compared to v3
could elaborate in what order .instance_post_init() hooks
will be called in this case for a Device?
(I mean compat_props_post_init vs device_post_init)

> +    };
> +
> +    type_register_static(&cp_interface_info);
> +}
> +
> +type_init(compat_props_register_types)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 3b31b2c025..b0ee05f837 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -970,28 +970,8 @@ static void device_initfn(Object *obj)
>      QLIST_INIT(&dev->gpios);
>  }
>  
> -static const GPtrArray *ac_compat_props;
> -static const GPtrArray *mc_compat_props;
> -
> -void accel_register_compat_props(const GPtrArray *props)
> -{
> -    ac_compat_props = props;
> -}
> -
> -void machine_register_compat_props(const GPtrArray *props)
> -{
> -    mc_compat_props = props;
> -}
> -
>  static void device_post_init(Object *obj)
>  {
> -    if (ac_compat_props) {
> -        object_apply_global_props(obj, ac_compat_props, &error_abort);
> -    }
> -    if (mc_compat_props) {
> -        object_apply_global_props(obj, mc_compat_props, &error_abort);
> -    }
> -
>      qdev_prop_set_globals(DEVICE(obj));
>  }
>  
> @@ -1124,6 +1104,10 @@ static const TypeInfo device_type_info = {
>      .class_init = device_class_init,
>      .abstract = true,
>      .class_size = sizeof(DeviceClass),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_COMPAT_PROPS },
> +        { }
> +    }
>  };
>  
>  static void qdev_register_types(void)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9410bbb7cf..adff09627f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1145,6 +1145,7 @@ Machine core
>  M: Eduardo Habkost <ehabkost@redhat.com>
>  M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>  S: Supported
> +F: hw/core/compat-props.c
>  F: hw/core/machine.c
>  F: hw/core/null-machine.c
>  F: include/hw/boards.h
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index a799c83815..f15b3c970a 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,5 +1,6 @@
>  # core qdev-related obj files, also used by *-user:
>  common-obj-y += qdev.o qdev-properties.o
> +common-obj-y += compat-props.o
>  common-obj-y += bus.o reset.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index fb0b449c02..fc74358c0a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -567,6 +567,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
>  	hw/core/irq.o \
>  	hw/core/fw-path-provider.o \
>  	hw/core/reset.o \
> +	hw/core/compat-props.o \
>  	$(test-qapi-obj-y)
>  tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
>  	migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \