Re: [Qemu-devel] [PATCH for-3.2 v4 16/28] hw: apply machine compat properties without touching globals

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 30 Nov 2018 01:36:03 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:  
> > > Hi
> > > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <imammedo@redhat.com> wrote:  
> > > >
> > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >  
> > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau wrote:  
> > > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > > > > >
> > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau wrote:  
> > > > > > > > Similarly to accel properties, move compat properties out of globals
> > > > > > > > registration, and apply the machine compat properties during
> > > > > > > > device_post_init().
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
> > > > > > > [...]  
> > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > > --- a/hw/core/qdev.c
> > > > > > > > +++ b/hw/core/qdev.c
> > > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object *obj)
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static const GPtrArray *ac_compat_props;
> > > > > > > > +static const GPtrArray *mc_compat_props;  
> > > > why you didn't use just 'compat_props' for both?
> > > > (it would be cleaner have single registry for compat
> > > > properties, and the place that takes care of registration
> > > > will take care of necessary ordering)  
> > >
> > > There are two arrays, one from the accelerator class, the other from
> > > the machine class. We can't make it a singleton (all compats props for
> > > the various machines would be mixed).
> > >
> > > We could create a third array that would be the set of both, but that
> > > would require more copy/allocation.  
> >
> > I am failing to see the advantage of replacing the `global_props`
> > static variable from qdev-properties.c with a collection of
> > separate static variables scattered around the code.  I thought
> > the main point of the changes was to reduce the amount of
> > duplicate data stored in static variables.
Main point was to use compats for backends then on top of that 
we added split global from compat properties goal.

> > I was expecting something like this:
> >
> > accel.c:
> >
> >   void accel_apply_compat_props(AccelState *accel, Object *obj)
> >   {
> >       object_apply_global_props(obj, ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> >   }
> >
> > machine.c:
> >
> >   /* Apply compat properties and global properties to an object */
> >   void machine_apply_compat_props(MachineState *ms, Object *obj)
> >   {
> >       accel_apply_compat_props(ms->accel, obj);
> >       object_apply_global_props(obj, MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> >   }
> >
> > compat-props.c:
> >
> >   static void object_apply_compat_props(Object *obj)
> >   {
> >       MachineState *machine = MACHINE(qdev_get_machine());
> >       machine_apply_compat_props(machine, obj);
> >   }
> >
> > 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(...)
> >   }
> >  
> 
> I like that solution too (which you also proposed in the other
> thread). But we have to decide whether it's acceptable to reference
> MachineState, or if the compat properties should be registered.
I dislike pulling in machine into basic object code and
I think a separate compats would be cleaner interface without
layer violation. But to unstuck, lets go with qdev_get_machine()
for now.

 
> --
> Marc-André Lureau
>