Re: [Qemu-devel] [PATCH v2] qom: Introduce object_realize_nofail()

[Anthony Liguori]

On 04/12/2012 04:08 PM, Andreas Färber wrote:
> Am 12.04.2012 21:59, schrieb Anthony Liguori:
> > On 04/12/2012 11:52 AM, Andreas Färber wrote:
> > > Am 12.04.2012 17:41, schrieb Anthony Liguori:
> > > > On 04/12/2012 09:04 AM, Andreas Färber wrote:
> > > > > Am 12.04.2012 16:59, schrieb Paolo Bonzini:
> > > > > > Il 12/04/2012 16:47, Anthony Liguori ha scritto:
> > > > > > > > Wrap setting of Object::realized property, error reporting and
> > > > > > > > exit(1)
> > > > > > > > into a helper function. It is the equivalent of qdev_init_nofail().
> > > > > > >
> > > > > > > I don't like this.
> > > > > > >
> > > > > > > If for no reason other than, a much more specific justification is
> > > > > > > needed for this.  I absolutely don't want to repeat the error
> > > > > > > handling
> > > > > > > mistakes of qdev.  I would rather we refactor all of the users of
> > > > > > > qdev_init_nofail() to propagate errors.
> > > > > >
> > > > > > I agree about this in general, but for a different reason.  There
> > > > > > should be
> > > > > > only one call to object_realize_nofail, in vl.c, which might as
> > > > > > well be
> > > > > > inlined---I'll include it in my series.  All calls to qdev_init_nofail
> > > > > > and qdev_init should disappear from boards that are properly converted
> > > > > > to QOM.
> > > > >
> > > > > We had talked about this on IRC and found that it won't work for
> > > > > selecting the type of an object.
> > > >
> > > > Can you be more specific?
> > > >
> > > > Selecting the type of an object should be done by having a link<>
> > > > property and letting the user create an object and setup the link.
> > >
> > > No, that's not what a link does. A link<>   property lets the user
> > > associate one instance, not a type that can be used for multiple
> > > instances. We'd need a template<>   or clone<>   mechanism for that!
> >
> > No, you misunderstood.  A link allows a user to create an object and
> > make an associate.  If you want to let a user choose a type, just have
> > them make an object.  Since you need no parameters to create an object,
> > there's no advantage of telling something about a type name vs. giving
> > it an object of that type.
>
> You're missing the point: You are advocating putting all configuration
> into a config file, having the user call --readconfig
> /path/to/board.conf and either fiddling with the config file or fiddling
> with (looping) QMP scripts to apply machine-wide changes.
>
> By contrast I am talking about backwards-compatible convenience options
> that influence more than one object in such a machine, wherever that
> hierarchy is being built up.

I think you're trying to run before we walk.

I don't want the user to call --readconfig.  I want to leave QEMUMachine alone 
for now.

I think we should focus on converting the core devices to a two stage 
initialization and then make use of that in the appropriate machine init function.

We don't need to convert every device to realize the benefits of this.  We just 
have to convert all the devices used by a single machine type, then we can 
introduce a --no-machine option and let the user avoid dealing with machines at all.

> Reality with SysBus is multi-stage constructors:
> A = qdev_create()
> A.a = x
> qdev_init_nofail(A) ->  A_initfn() ->  B = qdev_create()
>                                       B.b = y
>                                       qdev_init_nofail(B) ->  B_initfn()
> ...
>
> while you are dreaming of a flat hierarchy:

I'm not dreaming.  It's not a tremendous amount of work.

> A = qdev_create()
> B = qdev_create()
> ...
> A.a = x
> B.b = y
> ...
> object_property_set_bool(qdev_get_machine(), "realized", true)
>
> But getting there requires more than denying me object_realize(), it
> requires rewriting all qdev devices *first*, which I do not see Paolo
> doing. You started for i440fx but did not finish either.

Heh, you can't complain that I'm changing too much infrastructure too quickly 
and then complain that I'm not changing it quickly enough :-)  From the 
beginning I stated a clear goal for 1.1, that was core conversion and bus state. 
 I'll post bus state patches real soon (I think I posted them once already). 
We should have no problem hitting 1.1.

The end goal shouldn't be s/qdev/object/g.  That doesn't make things better by 
just using a new shiny infrastructure.  Splitting device initialization into two 
stages is the fundamental reason for introducing QOM in the first place.

It will allow us to introspect the full device model and then regenerate the 
same machine based on that introspection.  This is the fundamental problem to 
solve.  We shouldn't be trying to avoid it--it's the real end goal.

> Calling realize recursively shouldn't hurt us if realizing an already
> realized object is no-op and if the child iteration can cope with a
> growing child list for the current parent and any node in /machine.
> Meaning we can't just call one inlined object_realize_nofail() but we'd
> need to loop doing that until no node in the subgraph is left that is
> not realized.

I guess I don't understand what the problem we're trying to solve is.

Why can't we introduce an Object::realize() and just have it not automatically 
call DeviceState::init()?  That way we have a way to clearly indicate whether a 
device needs to be refactored.

Regards,

Anthony Liguori

> Andreas