[Qemu-devel] [PATCH 0/3] Fix some memory leaks caused by qemu_allocate_irqs

[Qemu-devel] [PATCH 0/3] Fix some memory leaks caused by qemu_allocate_irqs

[Stefan Weil]

There are several memory leaks in QEMU which are caused by calling
qemu_allocate_irqs.

Some of these leaks are fixed here in the 2nd and 3rd patch.
More can be fixed using the same pattern.

The first patch adds a new function which avoids unnecessary
memory allocation.

[PATCH 1/3] irq: Add new function qemu_init_irqs
[PATCH 2/3] hw/arm_timer: Fix memory leak (detected by Valgrind)
[PATCH 3/3] hw/spitz: Fix memory leaks (detected by Valgrind)

Regards

Stefan Weil

[Qemu-devel] [PATCH 1/3] irq: Add new function qemu_init_irqs

[Stefan Weil]

It is used to avoid dynamic memory allocation for qemu_irq arrays
with known size or single qemu_irq variables.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/irq.c |   15 +++++++++------
 hw/irq.h |    4 ++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/irq.c b/hw/irq.c
index d413a0b..fd284b0 100644
--- a/hw/irq.c
+++ b/hw/irq.c
@@ -38,14 +38,11 @@ void qemu_set_irq(qemu_irq irq, int level)
     irq->handler(irq->opaque, irq->n, level);
 }
 
-qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
+void qemu_init_irqs(qemu_irq_handler handler, void *opaque,
+                    qemu_irq *s, int n)
 {
-    qemu_irq *s;
-    struct IRQState *p;
     int i;
-
-    s = (qemu_irq *)g_malloc0(sizeof(qemu_irq) * n);
-    p = (struct IRQState *)g_malloc0(sizeof(struct IRQState) * n);
+    struct IRQState *p = g_new0(struct IRQState, n);
     for (i = 0; i < n; i++) {
         p->handler = handler;
         p->opaque = opaque;
@@ -53,6 +50,12 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
         s[i] = p;
         p++;
     }
+}
+
+qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
+{
+    qemu_irq *s = g_new(qemu_irq, n);
+    qemu_init_irqs(handler, opaque, s, n);
     return s;
 }
 
diff --git a/hw/irq.h b/hw/irq.h
index 56c55f0..368b88f 100644
--- a/hw/irq.h
+++ b/hw/irq.h
@@ -23,6 +23,10 @@ static inline void qemu_irq_pulse(qemu_irq irq)
     qemu_set_irq(irq, 0);
 }
 
+/* Initialize an array of N IRQs. */
+void qemu_init_irqs(qemu_irq_handler handler, void *opaque,
+                    qemu_irq *irqs, int n);
+
 /* Returns an array of N IRQs.  */
 qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n);
 void qemu_free_irqs(qemu_irq *s);
-- 
1.7.10

Re: [Qemu-devel] [PATCH 1/3] irq: Add new function qemu_init_irqs

[Peter Maydell]

On 24 September 2012 20:08, Stefan Weil <sw@weilnetz.de> wrote:
> It is used to avoid dynamic memory allocation for qemu_irq arrays
> with known size or single qemu_irq variables.

This patch is going to collide with Peter Crosthwaite's patch
that allows an irq array to be extended.

Also, the memory allocated with qemu_allocate_irqs() can be
freed with qemu_free_irqs(), but you don't seem to have
provided any way for direct callers of qemu_init_irqs()
to free the allocated IRQState array.

It feels to me like it ought to be possible to avoid having
random free-floating qemu_irq arrays at all : they should
all belong to devices which have (in theory) an opportunity
to free them in a deinit function. Explicit copying of qemu_irqs
out of one array and into other places, in particular, suggests
that maybe the code should be using qdev_get_gpio_in and
qdev_connect_gpio_out instead.

-- PMM

[Qemu-devel] [PATCH 2/3] hw/arm_timer: Fix memory leak (detected by Valgrind)

[Stefan Weil]

qi was allocated using g_malloc and never released.

It is not necessary to use dynamic memory allocation
for qi because it is copied to entries in s->timer
and not used otherwise.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/arm_timer.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm_timer.c b/hw/arm_timer.c
index e3ecce2..9e5f892 100644
--- a/hw/arm_timer.c
+++ b/hw/arm_timer.c
@@ -269,9 +269,8 @@ static const VMStateDescription vmstate_sp804 = {
 static int sp804_init(SysBusDevice *dev)
 {
     sp804_state *s = FROM_SYSBUS(sp804_state, dev);
-    qemu_irq *qi;
-
-    qi = qemu_allocate_irqs(sp804_set_irq, s, 2);
+    qemu_irq qi[2];
+    qemu_init_irqs(sp804_set_irq, s, qi, ARRAY_SIZE(qi));
     sysbus_init_irq(dev, &s->irq);
     s->timer[0] = arm_timer_init(s->freq0);
     s->timer[1] = arm_timer_init(s->freq1);
-- 
1.7.10

[Qemu-devel] [PATCH 3/3] hw/spitz: Fix memory leaks (detected by Valgrind)

[Stefan Weil]

outsignals was allocated using g_malloc and never released.
It is not necessary to use dynamic memory allocation
for that array because its entries are copied
and not used otherwise. Only 7 entries are used.

lcd_hsync can be initialized directly instead of copying the first entry
of a dynamically allocated array which is never released.

Cc: Andrzej Zaborowski <balrogg@gmail.com>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/spitz.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/spitz.c b/hw/spitz.c
index 20e7835..90dc35b 100644
--- a/hw/spitz.c
+++ b/hw/spitz.c
@@ -798,7 +798,8 @@ static void spitz_out_switch(void *opaque, int line, int level)
 static void spitz_scoop_gpio_setup(PXA2xxState *cpu,
                 DeviceState *scp0, DeviceState *scp1)
 {
-    qemu_irq *outsignals = qemu_allocate_irqs(spitz_out_switch, cpu, 8);
+    qemu_irq outsignals[7];
+    qemu_init_irqs(spitz_out_switch, cpu, outsignals, ARRAY_SIZE(outsignals));
 
     qdev_connect_gpio_out(scp0, SPITZ_SCP_CHRG_ON, outsignals[0]);
     qdev_connect_gpio_out(scp0, SPITZ_SCP_JK_B, outsignals[1]);
@@ -842,7 +843,7 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
      * wouldn't guarantee that a guest ever exits the loop.
      */
     spitz_hsync = 0;
-    lcd_hsync = qemu_allocate_irqs(spitz_lcd_hsync_handler, cpu, 1)[0];
+    qemu_init_irqs(spitz_lcd_hsync_handler, cpu, &lcd_hsync, 1);
     pxa2xx_gpio_read_notifier(cpu->gpio, lcd_hsync);
     pxa2xx_lcd_vsync_notifier(cpu->lcd, lcd_hsync);
 
-- 
1.7.10