On Tue, Oct 30, 2018 at 07:04:48PM +0400, Marc-André Lureau wrote:
> Handle calls of object_property_set_globals() with any object type,
> but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
>
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> qom/globals.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/qom/globals.c b/qom/globals.c
> index 587f4a1b5c..8664baebe0 100644
> --- a/qom/globals.c
> +++ b/qom/globals.c
> @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
>
> void object_property_set_globals(Object *obj)
> {
> - DeviceState *dev = DEVICE(obj);
> GList *l;
> + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> +
> + if (!dev && !IS_USER_CREATABLE(obj)) {
> + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> + return;
> + }
This is core QOM code, ideally type-specific code doesn't belong
here.
This also duplicates the purpose of the ObjectClass::set_globals
flag you have added on another patch, doesn't it? I suggest just
dropping this hunk, and letting callers decide if it's
appropriate to call object_property_set_globals() or not.
>
> for (l = global_props; l; l = l->next) {
> GlobalProperty *prop = l->data;
> Error *err = NULL;
>
> - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> continue;
> }
> prop->used = true;
> - object_property_parse(OBJECT(dev), prop->value, prop->property,
> &err);
> + object_property_parse(obj, prop->value, prop->property, &err);
> if (err != NULL) {
> error_prepend(&err, "can't apply global %s.%s=%s: ",
> prop->driver, prop->property, prop->value);
> - if (!dev->hotplugged && prop->errp) {
> +
> + if (dev && !dev->hotplugged && prop->errp) {
Hmm, more type-specific code. Can't we get rid of the
dev->hotplugged check here?
Maybe changing the function signature to:
void object_property_set_globals(Object *obj, bool propagate_errors);
and let the caller decide?
Or we could try to find a way to get rid of prop->errp. I never
really liked that hack, anyway.
Anyway, I won't mind keeping this code as-is if the solution is
too complex.
> error_propagate(prop->errp, err);
> } else {
> assert(prop->user_provided);
> @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> continue;
> }
> oc = object_class_by_name(prop->driver);
> - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> - if (!oc) {
> + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
This could use the ObjectClass::set_globals flag you have added.
> 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) {
> +
> + if (dc && !dc->hotpluggable) {
I wonder how we could get rid of this type-specific check. Maybe
a ObjectClass::only_init_time_globals flag, automatically
initialized by TYPE_DEVICE based on DeviceClass::hotpluggable?
In this case, I'm not sure it would be worth the extra
complexity. The type-specific code is just for a warning,
anyway, so it's not a big deal.
> warn_report("global %s.%s=%s not used",
> prop->driver, prop->property, prop->value);
> ret = 1;
> --
> 2.19.0.271.gfe8321ec05
>
>
--
Eduardo
