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

[Eduardo Habkost]

On Fri, Nov 30, 2018 at 11:55:26AM +0100, Igor Mammedov wrote:
> On Fri, 30 Nov 2018 01:36:03 +0400
> Marc-André Lureau <address@hidden> wrote:
> 
> > Hi
> > 
> > On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <address@hidden> 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 <address@hidden> wrote:  
> > > > >
> > > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > > Eduardo Habkost <address@hidden> 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 <address@hidden> 
> > > > > > > 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 <address@hidden>  
> > > > > > > > [...]  
> > > > > > > > > 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.

Which basic object code?  The only reference to machine above is
at compat-props.c.  I don't see any layering violation here.

-- 
Eduardo