Re: [PATCH v2 1/2] qom/object, qdev: move globals functions to object.c

[Daniel P . Berrangé]

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