| Message ID | 20160719085432.4572-27-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
> From: Marc-André Lureau [mailto:marcandre.lureau@gmail.com] > Sent: Thursday, 21 July 2016 4:15 > Hi Andrew, > > Since you introduced the timer, could you review this patch? > > thanks > > > ---------- Forwarded message ---------- > From: <marcandre.lureau@redhat.com> > Date: Tue, Jul 19, 2016 at 12:54 PM > Subject: [Qemu-devel] [PATCH 26/37] sd: free timer > To: qemu-devel@nongnu.org > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Free the timer allocated in instance_init. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/sd/sd.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 87c6dc1..8e88e83 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -1876,6 +1876,14 @@ static void sd_instance_init(Object *obj) > sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, > sd_ocr_powerup, sd); } > > +static void sd_instance_finalize(Object *obj) { > + SDState *sd = SD_CARD(obj); > + > + timer_del(sd->ocr_power_timer); > + timer_free(sd->ocr_power_timer); > +} > + > static void sd_realize(DeviceState *dev, Error **errp) { > SDState *sd = SD_CARD(dev); > @@ -1927,6 +1935,7 @@ static const TypeInfo sd_info = { > .class_size = sizeof(SDCardClass), > .class_init = sd_class_init, > .instance_init = sd_instance_init, > + .instance_finalize = sd_instance_finalize, > }; > > static void sd_register_types(void) Thanks for the fix. This was based on some other timer code I found in the tree that was evidently also leaky (I don't remember where unfortunately). One thing: are you sure it is safe to call timer_del() again if the timer may already have been deleted? It looks that way from the implementation, but the header comment isn't explicit. Otherwise, Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com> Cheers, Andrew
Hi On Thu, Jul 21, 2016 at 8:53 PM, Andrew Baumann <Andrew.Baumann@microsoft.com> wrote: > Thanks for the fix. This was based on some other timer code I found in the tree that was evidently also leaky (I don't remember where unfortunately). > > One thing: are you sure it is safe to call timer_del() again if the timer may already have been deleted? It looks that way from the implementation, but the header comment isn't explicit. Yes, it is safe, afaik, it's removing it from the list of active timers. > Otherwise, > Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com> thanks
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 87c6dc1..8e88e83 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1876,6 +1876,14 @@ static void sd_instance_init(Object *obj) sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd); } +static void sd_instance_finalize(Object *obj) +{ + SDState *sd = SD_CARD(obj); + + timer_del(sd->ocr_power_timer); + timer_free(sd->ocr_power_timer); +} + static void sd_realize(DeviceState *dev, Error **errp) { SDState *sd = SD_CARD(dev); @@ -1927,6 +1935,7 @@ static const TypeInfo sd_info = { .class_size = sizeof(SDCardClass), .class_init = sd_class_init, .instance_init = sd_instance_init, + .instance_finalize = sd_instance_finalize, }; static void sd_register_types(void)