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.
>
> > > > >
> > > > > void accel_register_compat_props(const GPtrArray *props)
> > > > > {
> > > > > ac_compat_props = props;
> > > > > }
> > > > >
> > > > > +void machine_register_compat_props(const GPtrArray *props)
> > > > > +{
> > > > > + mc_compat_props = props;
> > > > > +}
> > > > > +
> > > > > static void device_post_init(Object *obj)
> > > > > {
> > > > > if (ac_compat_props) {
> > > > > object_apply_global_props(obj, ac_compat_props,
> > > > > &error_abort);
> > > > > }
> > > >
> > > > Why not just use MACHINE(qdev_get_machine())->accel->compat_props
> > > > directly?
> > > >
> > > > > + if (mc_compat_props) {
> > > > > + object_apply_global_props(obj, mc_compat_props,
> > > > > &error_abort);
> > > > > + }
> > > >
> > > > Why not just use MACHINE(qdev_get_machine())->compat_props
> > > > directly?
> > >
> > > This was the approach in v3, but Igor didn't quite like referencing
> > > machine in qdev:
> > > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg04774.html
> >
> > I disagree with Igor, here. Core qdev code already have multiple
> > references to machine, I don't see any problem with that.
> (There are only 3 calls to qdev_get_machine() in core qdev.c
> blame me for adding one there. Which were hacks so we won't
> have to re-factor core qdev code. But that doesn't justify adding more.)
>
> This patch is an interim one and later in 25/28
> device_post_init() content is moved to a more generic compat interface
> implementation. That intended for use with types derived from Object
> (i.e. not only qdev stuff). Hence I'd like to decouple it from
> machine as a standalone feature as much as possible. So that
> machine (or whatever else) will opt in in using facility.
>
> > The previous code was clearer and easier to follow, and wasn't
> > sensitive to subtle changes in initialization ordering (e.g. what
> > happens if we create a device before *_register_compat_props() is
> > called?).
> Indeed It seems clearer to follow (that was my first impression as well),
> until I went through whole series and thought it's basically the same,
> So my choice was to use cleaner approach that we won't have to rewrite
> in near future.
>
> Thanks for bringing up ordering issue, we probably have one in this series.
>
> But beside possible issue here, even with v3 variant we would still have
> issues if objects are created before machine and accelerator instances are
> created.
> More correct way could be to register compat properties right away at
> select_machine() time, we don't really need an instance for that, just access
> to machine_class and do the same for 'accel' option. (that's probably doable
> within this series) + some time later (on top of this series) a check that
> no TYPE_COMPAT_PROPS were created at the moment compat properties are
> registered
> so we would notice when we write something wrong.
ok, I can look at that
>
> If it's too much of refactoring (series is already big as it is), I would
> compromise on qdev_get_machine() and adding TODO comments (or a series on top)
> to make it correct and "race-resistant".
>
> Marc are you sure it actually will work as expected with Object derived types?
> register_global_properties()
> is being called after
> qemu_opts_foreach(... user_creatable_add_opts_foreach,
> object_create_initial ...)
> so there is no compat properties registered when objects are created.
Good point, but in the case of hostmem, it works because
object_create_initial delays its creation.
> > >
> > > >
> > > > >
> > > > > qdev_prop_set_globals(DEVICE(obj));
> > > > > }
> > > > [...]
> > > >
> > > > --
> > > > Eduardo
> >
>
>
--
Marc-André Lureau
