Peter Maydell <peter.mayd...@linaro.org> writes: > On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <phi...@redhat.com> > wrote: >> >> On 2/17/20 2:25 PM, Peter Maydell wrote: > >> > So we now call timer_new in realize, and timer_del in unrealize, >> > but timer_free in finalize. This seems unbalanced -- why not >> > call timer_free in unrealize too, if we're moving things? >> > >> > Also, this is a case of code that's actually doing things right: >> > we free the memory that we allocated in init in finalize. So >> > we're not fixing any leak here, we're just moving code around >> > (which is reasonable if we're trying to standardize on "call >> > timer_new in realize, not init", but should be noted in the >> > commit message). >> >> While I understand your point, I am confused because the documentation >> on unrealize() and finalize() is rather scarce (and not obvious for >> non-native english speaker). I think I'm not understanding QOM instance >> lifetime well (in particular hotplug devices) so I will let this series go. > > Yes, the documentation is really not good at all. The > basic structure as I understand it is that we have two-part > creation and two-part destruction: > * instance_init is creation part 1 > * realize is creation part 2 > * unrealize is destruction part 1 and is the opposite of realize > * instance_finalize is destruction part 2 and is the > opposite of instance_init > > (Base QOM objects only have instance_init/instance_finalize; > realize/unrealize exists only for DeviceState objects > and their children.)
The split exists so you can set property values between instance_init() and realize(). It's how qdev has always worked. It permits setting properties one by one even when this results in intermediate states where invariants involving multiple property values are violated: delay checking them until realize(), rely on them only while the device is realized. Note that both realize() and unrealize() can fail. instance_init() and instance_finalize() can't. > ASCII-art state diagram: > > [start] --instance_init-> [inited] --realize-> [realized] > ^ | ^ | > \---instance_finalize---/ \-----unrealize-------/ > > In practice the only sequences we really care about are: > instance_init; realize; unrealize; instance_finalize > (a full object creation-use-destruction cycle; > even if realize fails, unrealize will be called) > instance_init; realize > (a subset of the above: for non-hot-pluggable devices > we will never try to unrealize them, so this is > as far as it goes for most devices unless they > returned an error from their realize function) > instance_init; instance_finalize > (the monitor does this for introspection of an object > without actually wanting to create and use it; it's > also the basic lifecycle for non-DeviceState objects) In theory, you can realize + unrealize multiple times. It might even work in practice sometimes. > The difference between hot-pluggable and not is just > whether it's valid to try to unrealize the device. > > We should definitely be clearer about what belongs in > instance_init vs what belongs in realize. But where we > have both a "do thing" and a "clean up that thing" task, > we should put the cleanup code in the operation that is > the pair of the operation we put the "do thing" code in > (i.e. do thing in instance_init, clean it up in finalize; > or do thing in realize, clean it up in unrealize). Not doing so risks introspection leaks or double-frees.
