Re: [Qemu-devel] [PATCH v2 05/10] qom/globals: generalize object_property_set_globals()

[Eduardo Habkost]

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 <address@hidden>
> ---
>  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