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

[Peter Maydell]

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.)

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)

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).

thanks
-- PMM