[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks
Date: Mon, 17 Feb 2020 14:48:11 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

On 2/17/20 2:25 PM, Peter Maydell wrote:
On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <address@hidden> 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é <address@hidden>
Cc: Pan Nengyuan <address@hidden>
  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);

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

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]