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é <berra...@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarb...@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 :|
