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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 31 Oct 2018 17:12:56 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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.lureau@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 v1, I've suggested to add a hook something like

ObjectClass::set_globals(Object *obj)

that will handle type specific code and probably
drop instance_post_init() hook.

that would be better than adding boolean ObjectClass::set_globals
as it could serve as both a flag and a type specific handler.
So in the end we would end up replacing instance_post_init()
with set_globals() hook.


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