Next patch will add Accel globals support. This means that globals won't be
qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.

Move all globals related functions and declarations to object.c. Each
function is renamed from 'qdev_' to 'object_':

- qdev_prop_register_global() is now object_prop_register_global()
- qdev_find_global_prop() is now object_find_global_prop()
- qdev_prop_check_globals() is now object_prop_check_globals()
- qdev_prop_set_globals() is now object_prop_set_globals()

For object_prop_set_globals() an additional change was made: the function
was hardwired to be used with DeviceState, where dev->hotplugged is checked
to determine if object_apply_global_props() will receive a NULL or an
&error_fatal errp. The function now receives an Object and an errp, and
logic using dev->hotplugged is moved to its caller (device_post_init()).

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/core/cpu-common.c                |  2 +-
 hw/core/qdev-properties-system.c    |  2 +-
 hw/core/qdev-properties.c           | 71 -----------------------------
 hw/core/qdev.c                      |  2 +-
 include/hw/qdev-core.h              | 27 -----------
 include/hw/qdev-properties.h        |  5 --
 include/qom/object.h                | 34 ++++++++++++++
 qom/object.c                        | 70 ++++++++++++++++++++++++++++
 system/vl.c                         |  6 +--
 target/i386/cpu.c                   |  2 +-
 target/sparc/cpu.c                  |  2 +-
 tests/unit/test-qdev-global-props.c |  4 +-
 12 files changed, 114 insertions(+), 113 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index f131cde2c0..794b18f7c5 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -182,7 +182,7 @@ static void cpu_common_parse_features(const char *typename, char *features,
             prop->driver = typename;
             prop->property = g_strdup(featurestr);
             prop->value = g_strdup(val);
-            qdev_prop_register_global(prop);
+            object_prop_register_global(prop);
         } else {
             error_setg(errp, "Expected key=value format, found %s.",
                        featurestr);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index f13350b4fb..5d30ee6257 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -41,7 +41,7 @@ static bool check_prop_still_unset(Object *obj, const char *name,
                                    const void *old_val, const char *new_val,
                                    bool allow_override, Error **errp)
 {
-    const GlobalProperty *prop = qdev_find_global_prop(obj, name);
+    const GlobalProperty *prop = object_find_global_prop(obj, name);
 
     if (!old_val || (!prop && allow_override)) {
         return true;
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 86a583574d..9cba33c311 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -855,77 +855,6 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
     qobject_unref(values);
 }
 
-static GPtrArray *global_props(void)
-{
-    static GPtrArray *gp;
-
-    if (!gp) {
-        gp = g_ptr_array_new();
-    }
-
-    return gp;
-}
-
-void qdev_prop_register_global(GlobalProperty *prop)
-{
-    g_ptr_array_add(global_props(), prop);
-}
-
-const GlobalProperty *qdev_find_global_prop(Object *obj,
-                                            const char *name)
-{
-    GPtrArray *props = global_props();
-    const GlobalProperty *p;
-    int i;
-
-    for (i = 0; i < props->len; i++) {
-        p = g_ptr_array_index(props, i);
-        if (object_dynamic_cast(obj, p->driver)
-            && !strcmp(p->property, name)) {
-            return p;
-        }
-    }
-    return NULL;
-}
-
-int qdev_prop_check_globals(void)
-{
-    int i, ret = 0;
-
-    for (i = 0; i < global_props()->len; i++) {
-        GlobalProperty *prop;
-        ObjectClass *oc;
-        DeviceClass *dc;
-
-        prop = g_ptr_array_index(global_props(), i);
-        if (prop->used) {
-            continue;
-        }
-        oc = object_class_by_name(prop->driver);
-        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
-        if (!oc) {
-            warn_report("global %s.%s has invalid class name",
-                        prop->driver, prop->property);
-            ret = 1;
-            continue;
-        }
-        dc = DEVICE_CLASS(oc);
-        if (!dc->hotpluggable && !prop->used) {
-            warn_report("global %s.%s=%s not used",
-                        prop->driver, prop->property, prop->value);
-            ret = 1;
-            continue;
-        }
-    }
-    return ret;
-}
-
-void qdev_prop_set_globals(DeviceState *dev)
-{
-    object_apply_global_props(OBJECT(dev), global_props(),
-                              dev->hotplugged ? NULL : &error_fatal);
-}
-
 /* --- 64bit unsigned int 'size' type --- */
 
 static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f3a996f57d..894372b776 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
      * precedence.
      */
     object_apply_compat_props(obj);
-    qdev_prop_set_globals(DEVICE(obj));
+    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5336728a23..656eb220f2 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -397,33 +397,6 @@ struct BusState {
     ResettableState reset;
 };
 
-/**
- * typedef GlobalProperty - a global property type
- *
- * @used: Set to true if property was used when initializing a device.
- * @optional: If set to true, GlobalProperty will be skipped without errors
- *            if the property doesn't exist.
- *
- * An error is fatal for non-hotplugged devices, when the global is applied.
- */
-typedef struct GlobalProperty {
-    const char *driver;
-    const char *property;
-    const char *value;
-    bool used;
-    bool optional;
-} GlobalProperty;
-
-static inline void
-compat_props_add(GPtrArray *arr,
-                 GlobalProperty props[], size_t nelem)
-{
-    int i;
-    for (i = 0; i < nelem; i++) {
-        g_ptr_array_add(arr, (void *)&props[i]);
-    }
-}
-
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
 /**
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 09aa04ca1e..301debfe79 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -206,11 +206,6 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
 
 void *object_field_prop_ptr(Object *obj, Property *prop);
 
-void qdev_prop_register_global(GlobalProperty *prop);
-const GlobalProperty *qdev_find_global_prop(Object *obj,
-                                            const char *name);
-int qdev_prop_check_globals(void);
-void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj,
                                     const char *name, const char *value);
 
diff --git a/include/qom/object.h b/include/qom/object.h
index 13d3a655dd..72ee237776 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -695,8 +695,42 @@ Object *object_new_with_propv(const char *typename,
                               Error **errp,
                               va_list vargs);
 
+/**
+ * typedef GlobalProperty - a global property type
+ *
+ * @used: Set to true if property was used when initializing an object
+ * @optional: If set to true, GlobalProperty will be skipped without errors
+ *            if the property doesn't exist.
+ *
+ * When used with devices: an error is fatal for non-hotplugged devices when
+ * the global is applied.
+ */
+typedef struct GlobalProperty {
+    const char *driver;
+    const char *property;
+    const char *value;
+    bool used;
+    bool optional;
+} GlobalProperty;
+
+static inline void
+compat_props_add(GPtrArray *arr,
+                 GlobalProperty props[], size_t nelem)
+{
+    int i;
+    for (i = 0; i < nelem; i++) {
+        g_ptr_array_add(arr, (void *)&props[i]);
+    }
+}
+
 bool object_apply_global_props(Object *obj, const GPtrArray *props,
                                Error **errp);
+void object_prop_set_globals(Object *obj, Error **errp);
+void object_prop_register_global(GlobalProperty *prop);
+int object_prop_check_globals(void);
+const GlobalProperty *object_find_global_prop(Object *obj,
+                                              const char *name);
+
 void object_set_machine_compat_props(GPtrArray *compat_props);
 void object_set_accelerator_compat_props(GPtrArray *compat_props);
 void object_register_sugar_prop(const char *driver, const char *prop,
diff --git a/qom/object.c b/qom/object.c
index 157a45c5f8..60dbd00edd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -472,6 +472,76 @@ bool object_apply_global_props(Object *obj, const GPtrArray *props,
     return true;
 }
 
+static GPtrArray *global_props(void)
+{
+    static GPtrArray *gp;
+
+    if (!gp) {
+        gp = g_ptr_array_new();
+    }
+
+    return gp;
+}
+
+void object_prop_register_global(GlobalProperty *prop)
+{
+    g_ptr_array_add(global_props(), prop);
+}
+
+void object_prop_set_globals(Object *obj, Error **errp)
+{
+    object_apply_global_props(obj, global_props(), errp);
+}
+
+const GlobalProperty *object_find_global_prop(Object *obj,
+                                              const char *name)
+{
+    GPtrArray *props = global_props();
+    const GlobalProperty *p;
+    int i;
+
+    for (i = 0; i < props->len; i++) {
+        p = g_ptr_array_index(props, i);
+        if (object_dynamic_cast(obj, p->driver)
+            && !strcmp(p->property, name)) {
+            return p;
+        }
+    }
+    return NULL;
+}
+
+int object_prop_check_globals(void)
+{
+    int i, ret = 0;
+
+    for (i = 0; i < global_props()->len; i++) {
+        GlobalProperty *prop;
+        ObjectClass *oc;
+        DeviceClass *dc;
+
+        prop = g_ptr_array_index(global_props(), i);
+        if (prop->used) {
+            continue;
+        }
+        oc = object_class_by_name(prop->driver);
+        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+        if (!oc) {
+            warn_report("global %s.%s has invalid class name",
+                        prop->driver, prop->property);
+            ret = 1;
+            continue;
+        }
+        dc = DEVICE_CLASS(oc);
+        if (!dc->hotpluggable && !prop->used) {
+            warn_report("global %s.%s=%s not used",
+                        prop->driver, prop->property, prop->value);
+            ret = 1;
+            continue;
+        }
+    }
+    return ret;
+}
+
 /*
  * Global property defaults
  * Slot 0: accelerator's global property defaults
diff --git a/system/vl.c b/system/vl.c
index bdd2f6ecf6..282668b817 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2153,7 +2153,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
     g->driver   = qemu_opt_get(opts, "driver");
     g->property = qemu_opt_get(opts, "property");
     g->value    = qemu_opt_get(opts, "value");
-    qdev_prop_register_global(g);
+    object_prop_register_global(g);
     return 0;
 }
 
@@ -2679,7 +2679,7 @@ static bool qemu_machine_creation_done(Error **errp)
         net_check_clients();
     }
 
-    qdev_prop_check_globals();
+    object_prop_check_globals();
 
     qdev_machine_creation_done();
 
@@ -3705,7 +3705,7 @@ void qemu_init(int argc, char **argv)
      * Beware, QOM objects created before this point miss global and
      * compat properties.
      *
-     * Global properties get set up by qdev_prop_register_global(),
+     * Global properties get set up by object_prop_register_global(),
      * called from user_register_global_props(), and certain option
      * desugaring.  Also in CPU feature desugaring (buried in
      * parse_cpu_option()), which happens below this point, but may
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4c2e6f3a71..b8838467a3 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5789,7 +5789,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
         prop->driver = typename;
         prop->property = g_strdup(name);
         prop->value = g_strdup(val);
-        qdev_prop_register_global(prop);
+        object_prop_register_global(prop);
     }
 
     if (ambiguous) {
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 9bacfb68cb..8dffa34701 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -114,7 +114,7 @@ cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
     prop->driver = typename;
     prop->property = g_strdup(name);
     prop->value = g_strdup(val);
-    qdev_prop_register_global(prop);
+    object_prop_register_global(prop);
 }
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string */
diff --git a/tests/unit/test-qdev-global-props.c b/tests/unit/test-qdev-global-props.c
index c8862cac5f..00a6b887f3 100644
--- a/tests/unit/test-qdev-global-props.c
+++ b/tests/unit/test-qdev-global-props.c
@@ -96,7 +96,7 @@ static void register_global_properties(GlobalProperty *props)
     int i;
 
     for (i = 0; props[i].driver != NULL; i++) {
-        qdev_prop_register_global(props + i);
+        object_prop_register_global(props + i);
     }
 }
 
@@ -235,7 +235,7 @@ static void test_dynamic_globalprop_subprocess(void)
 
     g_assert_cmpuint(mt->prop1, ==, 101);
     g_assert_cmpuint(mt->prop2, ==, 102);
-    global_error = qdev_prop_check_globals();
+    global_error = object_prop_check_globals();
     g_assert_cmpuint(global_error, ==, 1);
     g_assert(props[0].used);
     g_assert(props[1].used);
-- 
2.45.2

CC: Markus since he's had opinions on stuff related to -global  in
the past.

On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
> Next patch will add Accel globals support. This means that globals won't be
> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
> 
> Move all globals related functions and declarations to object.c. Each
> function is renamed from 'qdev_' to 'object_':
> 
> - qdev_prop_register_global() is now object_prop_register_global()
> - qdev_find_global_prop() is now object_find_global_prop()
> - qdev_prop_check_globals() is now object_prop_check_globals()
> - qdev_prop_set_globals() is now object_prop_set_globals()
> 
> For object_prop_set_globals() an additional change was made: the function
> was hardwired to be used with DeviceState, where dev->hotplugged is checked
> to determine if object_apply_global_props() will receive a NULL or an
> &error_fatal errp. The function now receives an Object and an errp, and
> logic using dev->hotplugged is moved to its caller (device_post_init()).
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/core/cpu-common.c                |  2 +-
>  hw/core/qdev-properties-system.c    |  2 +-
>  hw/core/qdev-properties.c           | 71 -----------------------------
>  hw/core/qdev.c                      |  2 +-
>  include/hw/qdev-core.h              | 27 -----------
>  include/hw/qdev-properties.h        |  5 --
>  include/qom/object.h                | 34 ++++++++++++++
>  qom/object.c                        | 70 ++++++++++++++++++++++++++++
>  system/vl.c                         |  6 +--
>  target/i386/cpu.c                   |  2 +-
>  target/sparc/cpu.c                  |  2 +-
>  tests/unit/test-qdev-global-props.c |  4 +-
>  12 files changed, 114 insertions(+), 113 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index f131cde2c0..794b18f7c5 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -182,7 +182,7 @@ static void cpu_common_parse_features(const char *typename, char *features,
>              prop->driver = typename;
>              prop->property = g_strdup(featurestr);
>              prop->value = g_strdup(val);
> -            qdev_prop_register_global(prop);
> +            object_prop_register_global(prop);
>          } else {
>              error_setg(errp, "Expected key=value format, found %s.",
>                         featurestr);
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index f13350b4fb..5d30ee6257 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -41,7 +41,7 @@ static bool check_prop_still_unset(Object *obj, const char *name,
>                                     const void *old_val, const char *new_val,
>                                     bool allow_override, Error **errp)
>  {
> -    const GlobalProperty *prop = qdev_find_global_prop(obj, name);
> +    const GlobalProperty *prop = object_find_global_prop(obj, name);
>  
>      if (!old_val || (!prop && allow_override)) {
>          return true;
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 86a583574d..9cba33c311 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -855,77 +855,6 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
>      qobject_unref(values);
>  }
>  
> -static GPtrArray *global_props(void)
> -{
> -    static GPtrArray *gp;
> -
> -    if (!gp) {
> -        gp = g_ptr_array_new();
> -    }
> -
> -    return gp;
> -}
> -
> -void qdev_prop_register_global(GlobalProperty *prop)
> -{
> -    g_ptr_array_add(global_props(), prop);
> -}
> -
> -const GlobalProperty *qdev_find_global_prop(Object *obj,
> -                                            const char *name)
> -{
> -    GPtrArray *props = global_props();
> -    const GlobalProperty *p;
> -    int i;
> -
> -    for (i = 0; i < props->len; i++) {
> -        p = g_ptr_array_index(props, i);
> -        if (object_dynamic_cast(obj, p->driver)
> -            && !strcmp(p->property, name)) {
> -            return p;
> -        }
> -    }
> -    return NULL;
> -}
> -
> -int qdev_prop_check_globals(void)
> -{
> -    int i, ret = 0;
> -
> -    for (i = 0; i < global_props()->len; i++) {
> -        GlobalProperty *prop;
> -        ObjectClass *oc;
> -        DeviceClass *dc;
> -
> -        prop = g_ptr_array_index(global_props(), i);
> -        if (prop->used) {
> -            continue;
> -        }
> -        oc = object_class_by_name(prop->driver);
> -        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> -        if (!oc) {
> -            warn_report("global %s.%s has invalid class name",
> -                        prop->driver, prop->property);
> -            ret = 1;
> -            continue;
> -        }
> -        dc = DEVICE_CLASS(oc);
> -        if (!dc->hotpluggable && !prop->used) {
> -            warn_report("global %s.%s=%s not used",
> -                        prop->driver, prop->property, prop->value);
> -            ret = 1;
> -            continue;
> -        }
> -    }
> -    return ret;
> -}
> -
> -void qdev_prop_set_globals(DeviceState *dev)
> -{
> -    object_apply_global_props(OBJECT(dev), global_props(),
> -                              dev->hotplugged ? NULL : &error_fatal);
> -}
> -
>  /* --- 64bit unsigned int 'size' type --- */
>  
>  static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f3a996f57d..894372b776 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
>       * precedence.
>       */
>      object_apply_compat_props(obj);
> -    qdev_prop_set_globals(DEVICE(obj));
> +    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
>  }

This is pretty awkward :-(

If we're generalizing this global properties concept, then we want
object_prop_set_globals to be called from the Object base class
code. We can't do that given this need to check the 'hotplugged'
property.

That check, however, is total insanity. Pre-existing problem,
not your fault.

I imagine the rationale is that we don't want to kill QEMU
if setting a global fails, and we're in middle of device_add
on a running VM.

Throwing away errors though is unacceptable IMHO. device_add
can report errors and we should be propagating them. Likewise
for object_add, or any object HMP command creating QOM types.

The trouble is that we're about 4-5 levels deep in a call
chain that lacks "Error **errp".

The root problem is that none of object_new, object_new_with_class
and object_new_with_type have a "Error *errp" parameter.

object_new_with_props and object_new_with_propv both *do* have
a "Error *errp" parameter, but then they call into object_new_with_type
and can't get errors back from that.

IMHO we need to fix this inability to report errors from object
construction. It will certainly be a painful refactoring job,
but I think its neccessary in order to support global props
without this horrible hack checking the "hotpluggable" flag.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> CC: Markus since he's had opinions on stuff related to -global  in
> the past.
>
> On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
>> Next patch will add Accel globals support. This means that globals won't be
>> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
>> 
>> Move all globals related functions and declarations to object.c. Each
>> function is renamed from 'qdev_' to 'object_':
>> 
>> - qdev_prop_register_global() is now object_prop_register_global()
>> - qdev_find_global_prop() is now object_find_global_prop()
>> - qdev_prop_check_globals() is now object_prop_check_globals()
>> - qdev_prop_set_globals() is now object_prop_set_globals()
>> 
>> For object_prop_set_globals() an additional change was made: the function
>> was hardwired to be used with DeviceState, where dev->hotplugged is checked
>> to determine if object_apply_global_props() will receive a NULL or an
>> &error_fatal errp. The function now receives an Object and an errp, and
>> logic using dev->hotplugged is moved to its caller (device_post_init()).
>> 
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

[...]

>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index f3a996f57d..894372b776 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
>>       * precedence.
>>       */
>>      object_apply_compat_props(obj);
>> -    qdev_prop_set_globals(DEVICE(obj));
>> +    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
>>  }
>
> This is pretty awkward :-(
>
> If we're generalizing this global properties concept, then we want
> object_prop_set_globals to be called from the Object base class
> code.

Yes.  This series copies the concept from devices to accelerators.  But
since it clearly makes sense for any kind of object, we better move it
to objects instead.  We may not be able to get there in one step,
though.

>       We can't do that given this need to check the 'hotplugged'
> property.
>
> That check, however, is total insanity. Pre-existing problem,
> not your fault.
>
> I imagine the rationale is that we don't want to kill QEMU
> if setting a global fails, and we're in middle of device_add
> on a running VM.

Yes.

> Throwing away errors though is unacceptable IMHO.

To be precise: we're silently ignoring any -global that fail to apply.

I agree that's wrong.

>                                                   device_add
> can report errors and we should be propagating them. Likewise
> for object_add, or any object HMP command creating QOM types.
>
> The trouble is that we're about 4-5 levels deep in a call
> chain that lacks "Error **errp".
>
> The root problem is that none of object_new, object_new_with_class
> and object_new_with_type have a "Error *errp" parameter.

This is a fundamental QOM design decision.

Not mine, mind.  Moreover, I wasn't there, so my idea on design
rationale may well be off; keep that in mind.

QOM properties are not declared statically, they are created
dynamically.  Aside: this is, in my not particularly humble opinion, a
spectacularly bad idea.

Properties are generally created in instance_init() methods.

Fine print: we later added "class properties", which are created
dynamically within the class, and cloned into the instance before 
its instance_init() method runs.

Object creation doesn't take arguments, and cannot fail.  An
instance_init() method doesn't take arguments, and cannot fail.

Objects are configured via properties.  Property setters take an
argument (the property value), and can fail.

Any part of object creation + configuration that could fail must be done
in property setters.

Common usage is create object, configure by setting properties, operate.

The state transition between "configuring" and "operating" is important.
For devices, this state transition happens when property "realized" is
set to true.  For user-creatable objects it happens when method
complete() is called.  Both can fail.  For everything else, the
transition is implicit / ad hoc / unclear.

For more on this (and other QOM design issues), see my memo "Dynamic &
heterogeneous machines, initial configuration: problems", in particular
section "Problem 5: QOM lacks a clear life cycle".
Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/

To introspect properties, you need an object.  You can always create one
for that (can't fail).  Properties created outside object initialization
cannot be introspected that way.  This is how qom-list-properties works.

> object_new_with_props and object_new_with_propv both *do* have
> a "Error *errp" parameter,

Yes, because they combine object creation, which cannot fail, with
setting properties, which can fail.

>                            but then they call into object_new_with_type
> and can't get errors back from that.
>
> IMHO we need to fix this inability to report errors from object
> construction. It will certainly be a painful refactoring job,
> but I think its neccessary in order to support global props
> without this horrible hack checking the "hotpluggable" flag.

Beyond painful.  Possibly infeasible.

Object creation cannot fail.  If we revise this fundamental QOM design
decision, we get to update all call chains leading to object creation.
That's a *massive* undertaking.

Can we solve the problems we have without revising QOM design?

We'd need to delay the actual failure to a point where the design admits
failure.

Here's an idea.  Formalize the life cycle, i.e. make it an explicit
state machine.  Add a "failed" state.  Any error during object creation
makes the object go to "failed".  Going from "failed" to "operating"
fails.  Which is fine, because the design admits failure there.

Thoughts?

On Thu, Aug 01, 2024 at 08:58:10AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > CC: Markus since he's had opinions on stuff related to -global  in
> > the past.
> >
> > On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
> >> Next patch will add Accel globals support. This means that globals won't be
> >> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
> >> 
> >> Move all globals related functions and declarations to object.c. Each
> >> function is renamed from 'qdev_' to 'object_':
> >> 
> >> - qdev_prop_register_global() is now object_prop_register_global()
> >> - qdev_find_global_prop() is now object_find_global_prop()
> >> - qdev_prop_check_globals() is now object_prop_check_globals()
> >> - qdev_prop_set_globals() is now object_prop_set_globals()
> >> 
> >> For object_prop_set_globals() an additional change was made: the function
> >> was hardwired to be used with DeviceState, where dev->hotplugged is checked
> >> to determine if object_apply_global_props() will receive a NULL or an
> >> &error_fatal errp. The function now receives an Object and an errp, and
> >> logic using dev->hotplugged is moved to its caller (device_post_init()).
> >> 
> >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
> [...]
> 
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index f3a996f57d..894372b776 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
> >>       * precedence.
> >>       */
> >>      object_apply_compat_props(obj);
> >> -    qdev_prop_set_globals(DEVICE(obj));
> >> +    object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL : &error_fatal);
> >>  }
> >
> > This is pretty awkward :-(
> >
> > If we're generalizing this global properties concept, then we want
> > object_prop_set_globals to be called from the Object base class
> > code.
> 
> Yes.  This series copies the concept from devices to accelerators.  But
> since it clearly makes sense for any kind of object, we better move it
> to objects instead.  We may not be able to get there in one step,
> though.

Looking at how 'DEVICE(obj)->hotplugged' gets set, this logic does
not really need to have a dependancy on Device.

  static void device_initfn(Object *obj)
  {
      DeviceState *dev = DEVICE(obj);

      if (phase_check(PHASE_MACHINE_READY)) {
          dev->hotplugged = 1;
      ...
  }

IOW, we could at least be doing

  object_prop_set_globals(obj, phase_check(PHASE_MACHINE_READY) ? NULL : &error_fatal);

if we wanted this logic in the Object base class, however....


> >                                                   device_add
> > can report errors and we should be propagating them. Likewise
> > for object_add, or any object HMP command creating QOM types.
> >
> > The trouble is that we're about 4-5 levels deep in a call
> > chain that lacks "Error **errp".
> >
> > The root problem is that none of object_new, object_new_with_class
> > and object_new_with_type have a "Error *errp" parameter.
> 
> This is a fundamental QOM design decision.
> 
> Not mine, mind.  Moreover, I wasn't there, so my idea on design
> rationale may well be off; keep that in mind.
> 
> QOM properties are not declared statically, they are created
> dynamically.  Aside: this is, in my not particularly humble opinion, a
> spectacularly bad idea.
> 
> Properties are generally created in instance_init() methods.
> 
> Fine print: we later added "class properties", which are created
> dynamically within the class, and cloned into the instance before 
> its instance_init() method runs.
> 
> Object creation doesn't take arguments, and cannot fail.  An
> instance_init() method doesn't take arguments, and cannot fail.
> 
> Objects are configured via properties.  Property setters take an
> argument (the property value), and can fail.
> 
> Any part of object creation + configuration that could fail must be done
> in property setters.
> 
> Common usage is create object, configure by setting properties, operate.
> 
> The state transition between "configuring" and "operating" is important.
> For devices, this state transition happens when property "realized" is
> set to true.  For user-creatable objects it happens when method
> complete() is called.  Both can fail.  For everything else, the
> transition is implicit / ad hoc / unclear.




> 
> For more on this (and other QOM design issues), see my memo "Dynamic &
> heterogeneous machines, initial configuration: problems", in particular
> section "Problem 5: QOM lacks a clear life cycle".
> Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
> 
> To introspect properties, you need an object.  You can always create one
> for that (can't fail).  Properties created outside object initialization
> cannot be introspected that way.  This is how qom-list-properties works.
> 
> > object_new_with_props and object_new_with_propv both *do* have
> > a "Error *errp" parameter,
> 
> Yes, because they combine object creation, which cannot fail, with
> setting properties, which can fail.
> 
> >                            but then they call into object_new_with_type
> > and can't get errors back from that.
> >
> > IMHO we need to fix this inability to report errors from object
> > construction. It will certainly be a painful refactoring job,
> > but I think its neccessary in order to support global props
> > without this horrible hack checking the "hotpluggable" flag.
> 
> Beyond painful.  Possibly infeasible.
> 
> Object creation cannot fail.  If we revise this fundamental QOM design
> decision, we get to update all call chains leading to object creation.
> That's a *massive* undertaking.
> 
> Can we solve the problems we have without revising QOM design?

Hmm, to cover  object_add QMP and -object CLI, we could just put
setting of global props into the object_new_with_prop{s,v} method,
which would deal with usage of user creatable objects, and a few
internal calls which handle errors.

device_add/-device won't hit that code path. Even if we converted
those over to object_new_with_prop{s,v}, that only handles the top
level device. Many devices will create sub-devices behind the scenes
and global properties are supposed to apply to those, and those will
all just be using plain qdev_new().

> We'd need to delay the actual failure to a point where the design admits
> failure.
> 
> Here's an idea.  Formalize the life cycle, i.e. make it an explicit
> state machine.  Add a "failed" state.  Any error during object creation
> makes the object go to "failed".  Going from "failed" to "operating"
> fails.  Which is fine, because the design admits failure there.

I guess the challenge is that we then need to be confident that the
state change is effected everywhere it needs to be, which sounds like
another bit of hairy work unless we can limit the scope.

We could do

  * Add globals support to  object_new_with_prop{s,v}
  * Convert Accel creation over to object_new_with_prop{s,v},
  * Records errors setting globals in device_post_init in a
    field in DeviceState
  * Make qdev_realize() check for & report the error from
    the step above, before doing its normal logic

IOW, going forward we have two patterns

  * Creation uses object_new_with_prop{s,v} and has globals
    set that way (preferred). Works for -object, -accel,
    object_add and some internal direct API calls

  * Creation uses a post-init hook, but the type has to have
    some state change equivalent to 'realized' to do delayed
    reporting of globals errors (discouraged, ideally only
    for Device). Works for -device, device_add

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|