Re: [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals()

[Marc-André Lureau]

Hi

On Mon, Oct 29, 2018 at 5:11 PM Igor Mammedov <address@hidden> wrote:
>
> On Wed, 12 Sep 2018 16:55:27 +0400
> Marc-André Lureau <address@hidden> 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;
> > +    }
> more dynamic casts but now imposed on every create object :(
>
> Maybe we should add ObjectClass::check/set_globals hook?
> It would be cheap to check and only objects we intend to work with
> it would be affected. On top of that hooks could be different so
> that device/user_creatable specifics won't be in generic code
> (like it's implemented here).

I don't think adding a few casts during object creation will impact
negatively in a measurable way.

However I'll add an additional patch to the series to implement what I
think you suggested.

thanks
>
> >
> >      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) {
> >                  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) {
> >              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) {
> >              warn_report("global %s.%s=%s not used",
> >                          prop->driver, prop->property, prop->value);
> >              ret = 1;
>