qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [Qemu-devel] [PATCH for-3.2 v4 16/28] hw: apply machine c


From: Marc-André Lureau
Subject: Re: [Qemu-arm] [Qemu-devel] [PATCH for-3.2 v4 16/28] hw: apply machine compat properties without touching globals
Date: Fri, 30 Nov 2018 01:36:03 +0400

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


--
Marc-André Lureau



reply via email to

[Prev in Thread] Current Thread [Next in Thread]