On Fri, 30 Nov 2018 01:36:03 +0400
Marc-André Lureau <marcandre.lur...@gmail.com> wrote:
> Hi
>
> On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <ehabk...@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 <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.
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
>
