Re: [Qemu-devel] [PATCH for-3.2 v4 25/28] machine: add compat-props interface

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Nov 30, 2018 at 01:34:56PM +0100, Igor Mammedov wrote:
> On Thu, 29 Nov 2018 15:49:00 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Nov 27, 2018 at 01:27:58PM +0400, Marc-André Lureau wrote:
> > > Let's make compatiblity properties an interface, so that objects other
> > > than QDev can benefit from having machine compatiblity properties.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
> > [...]
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 3b31b2c025..b0ee05f837 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -970,28 +970,8 @@ static void device_initfn(Object *obj)
> > >      QLIST_INIT(&dev->gpios);
> > >  }
> > >  
> > > -static const GPtrArray *ac_compat_props;
> > > -static const GPtrArray *mc_compat_props;
> > > -
> > > -void accel_register_compat_props(const GPtrArray *props)
> > > -{
> > > -    ac_compat_props = props;
> > > -}
> > > -
> > > -void machine_register_compat_props(const GPtrArray *props)
> > > -{
> > > -    mc_compat_props = props;
> > > -}
> > > -
> > >  static void device_post_init(Object *obj)
> > >  {
> > > -    if (ac_compat_props) {
> > > -        object_apply_global_props(obj, ac_compat_props, &error_abort);
> > > -    }
> > > -    if (mc_compat_props) {
> > > -        object_apply_global_props(obj, mc_compat_props, &error_abort);
> > > -    }
> > > -
> > >      qdev_prop_set_globals(DEVICE(obj));
> > >  }
> > >  
> > > @@ -1124,6 +1104,10 @@ static const TypeInfo device_type_info = {
> > >      .class_init = device_class_init,
> > >      .abstract = true,
> > >      .class_size = sizeof(DeviceClass),
> > > +    .interfaces = (InterfaceInfo[]) {
> > > +        { TYPE_COMPAT_PROPS },
> > > +        { }
> > > +    }  
> > 
> > At first I thought TYPE_COMPAT_PROPS was a practical way to
> > implement this feature, but now I'm worried: the ordering between
> > compat_props_post_init() qdev_prop_set_globals() is very
> > important (user-provided globals must always be set after compat
> > props), and here the ordering is implicit and easy to break
> > accidentally.
> > 
> > What if instead of a QOM interface we just provide a simple
> > object_apply_compat_props() function?  e.g.:
> > 
> > qdev.c:
> > 
> >   static void device_post_init(Object *obj)
> >   {
> >       object_apply_compat_props(obj);
> >       apply_user_provided_globals(obj);
> >   }
> > 
> > object_interface.c:
> > 
> >   void user_creatable_complete(Object *obj, Error **errp)
> >   {
> >       object_apply_compat_props(obj);
> >       ...
> >       ucc->complete(...)
> >   }
> this would work as long as nothing is happening between
> object_new() ... user_creatable_complete() but look at
> current users of user_creatable_complete() so it's
> fragile too.

You are right.  This could be solved by:

  void user_creatable_post_init(Object *obj)
  {
      object_apply_compat_props(obj);
  }


> 
> Reasons for compat props interface are the same as for
> instance_post_init/device_post_init.
> 
> the thing we can do here is getting rid of device_post_init
> and making device override TYPE_COMPAT_PROPS::instance_post_init
> to make explicit ordering like we do everywhere else:
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55..46ad6f5 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1033,11 +1033,19 @@ static void device_unparent(Object *obj)
>      }
>  }
>  
> +device_compat_props()
> +{
> +    dc->parent_compat_props()
> +    apply_global_props()
> +}
> +
>  static void device_class_init(ObjectClass *class, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(class);
>  
>      class->unparent = device_unparent;
> +    dc->parent_compat_props = COMPAT_PROPS_GET_CLASS(class)->instance_post_init
> +    COMPAT_PROPS_GET_CLASS(class)->instance_post_init = device_compat_props()
>  
>      /* by default all devices were considered as hotpluggable,
>       * so with intent to check it in generic qdev_unplug() /

The only advantage I saw in TYPE_COMPAT_PROPS interface was to
easily allow objects to implement behavior without manually
implementing post_init.  Now we can't use the interface without
an even more complex way of overriding post_init, so what's the
point?

Why not just call object_apply_compat_props() at
device_post_init()?

> 
> > Most people don't understand QOM interfaces and their
> > initialization ordering rules.  Everybody understands C function
> > calls.
> > 
> > > [...]  
> > 
> 

-- 
Eduardo