Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <address@hidden> 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.