On 2/17/20 2:25 PM, Peter Maydell wrote:
On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <phi...@redhat.com> wrote:In commit f3a508eb4e the Euler Robot reported calling timer_new() in instance_init() can leak heap memory. The easier fix is to delay the timer creation at instance realize(). Similarly move timer_del() into a new instance unrealize() method. This case was found with the following coccinelle script: @ match @ identifier instance_init; typedef Object; identifier obj; expression val, scale; identifier clock_type, callback, opaque; position pos; @@ static void instance_init(Object *obj) { <... ( val = timer_new@pos(clock_type, scale, callback, opaque); | val = timer_new_ns@pos(clock_type, callback, opaque); | val = timer_new_us@pos(clock_type, callback, opaque); | val = timer_new_ms@pos(clock_type, callback, opaque); ) ...> } @ script:python @ f << match.instance_init; p << match.pos; @@ print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f) Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> --- Cc: Pan Nengyuan <pannengy...@huawei.com> --- hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c index f9a13e0a44..9144ac6c38 100644 --- a/hw/ipmi/ipmi_bmc_extern.c +++ b/hw/ipmi/ipmi_bmc_extern.c @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp) qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive, chr_event, NULL, ibe, NULL, true); + + ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); +} + +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp) +{ + IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev); + + timer_del(ibe->extern_timer); } static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id) @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj) { IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); - ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe); vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe); } @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj) { IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj); - timer_del(ibe->extern_timer); timer_free(ibe->extern_timer); }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.
Similar confusions: * https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg03079.html Eduardo Habkost wrote: > Philippe Mathieu-Daudé wrote: >> IIUC when we use both init() and realize(), realize() should >> only contains on code that consumes the object properties... >> But maybe the design is not clear. Then why not move all the >> init() code to realize()? > > Normally I would recommend the opposite: delay as much as > possible to realize(), to avoid unwanted side effects when > (e.g.) running qom-list-properties. * https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04143.html David Hildenbrand wrote: >> Philippe Mathieu-Daudé wrote: >> Similarly to your other cleanups, shouldn't we move the >> timer_new_ns() into a realize() function, then do the >> timer_del() in unrealize()? > > include/hw/qdev-core.h > > "Trivial field initializations should go into > #TypeInfo.instance_init. Operations depending on @props > static properties should go into @realize."
thanks -- PMM
