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

[Igor Mammedov]

On Tue, 30 Oct 2018 16:16:43 +0400
Marc-André Lureau <address@hidden> wrote:

> 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.
When it's applied to every object created as opposed to -object CLI option,
it will add up. For example 512 x 2 (cpus + apic) ~ 1000 x cast_cost,
for pc-dimm 255 x 3 (backend + memory_region + pc-dimm) ~ 800 x cast_cost,
We can have more backends of other types, basically everything QOM will
incur tiny extra cost.
I'm just thinking about worst case scenario.


> However I'll add an additional patch to the series to implement what I
> think you suggested.
thanks,
for this patch also important to keep Device vs UserCreatable
parts separate instead of dumping evrything into one pit.

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