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

[Eduardo Habkost]

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 <imamm...@redhat.com> wrote:
> >
> > On Tue, 27 Nov 2018 11:35:27 -0200
> > Eduardo Habkost <ehabk...@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 <ehabk...@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.lur...@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.

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(...)
  }

-- 
Eduardo