diff mbox

[29/29] hw/s390x/sclpcpu.c: Fix memory leak spotted by valgrind

Message ID 1432814932-12608-30-git-send-email-zhaoshenglong@huawei.com
State New
Headers show

Commit Message

Shannon Zhao May 28, 2015, 12:08 p.m. UTC 
From: Shannon Zhao <shannon.zhao@linaro.org>

valgrind complains about:
==1413== 188 (8 direct, 180 indirect) bytes in 1 blocks are definitely lost in loss record 951 of 1,199
==1413==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==1413==    by 0x262D8B: malloc_and_trace (vl.c:2556)
==1413==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
==1413==    by 0x2C7ACF: qemu_extend_irqs (irq.c:55)
==1413==    by 0x2C7B5B: qemu_allocate_irqs (irq.c:64)
==1413==    by 0x2168F4: irq_cpu_hotplug_init (sclpcpu.c:84)
==1413==    by 0x21623F: event_realize (event-facility.c:385)
==1413==    by 0x2C4610: device_set_realized (qdev.c:1058)
==1413==    by 0x317DDA: property_set_bool (object.c:1514)
==1413==    by 0x3166D4: object_property_set (object.c:837)
==1413==    by 0x3189F6: object_property_set_qobject (qom-qobject.c:24)
==1413==    by 0x316943: object_property_set_bool (object.c:905)

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/s390x/sclpcpu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Cornelia Huck May 28, 2015, 1:11 p.m. UTC | #1 
On Thu, 28 May 2015 20:08:52 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> valgrind complains about:
> ==1413== 188 (8 direct, 180 indirect) bytes in 1 blocks are definitely lost in loss record 951 of 1,199
> ==1413==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==1413==    by 0x262D8B: malloc_and_trace (vl.c:2556)
> ==1413==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==1413==    by 0x2C7ACF: qemu_extend_irqs (irq.c:55)
> ==1413==    by 0x2C7B5B: qemu_allocate_irqs (irq.c:64)
> ==1413==    by 0x2168F4: irq_cpu_hotplug_init (sclpcpu.c:84)
> ==1413==    by 0x21623F: event_realize (event-facility.c:385)
> ==1413==    by 0x2C4610: device_set_realized (qdev.c:1058)
> ==1413==    by 0x317DDA: property_set_bool (object.c:1514)
> ==1413==    by 0x3166D4: object_property_set (object.c:837)
> ==1413==    by 0x3189F6: object_property_set_qobject (qom-qobject.c:24)
> ==1413==    by 0x316943: object_property_set_bool (object.c:905)
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/s390x/sclpcpu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
> index 2fe8b5a..1090e2f 100644
> --- a/hw/s390x/sclpcpu.c
> +++ b/hw/s390x/sclpcpu.c
> @@ -25,13 +25,13 @@ typedef struct ConfigMgtData {
>      uint8_t event_qualifier;
>  } QEMU_PACKED ConfigMgtData;
> 
> -static qemu_irq *irq_cpu_hotplug; /* Only used in this file */
> +static qemu_irq irq_cpu_hotplug; /* Only used in this file */
> 
>  #define EVENT_QUAL_CPU_CHANGE  1
> 
>  void raise_irq_cpu_hotplug(void)
>  {
> -    qemu_irq_raise(*irq_cpu_hotplug);
> +    qemu_irq_raise(irq_cpu_hotplug);
>  }
> 
>  static unsigned int send_mask(void)
> @@ -81,7 +81,10 @@ static void trigger_signal(void *opaque, int n, int level)
> 
>  static int irq_cpu_hotplug_init(SCLPEvent *event)
>  {
> -    irq_cpu_hotplug = qemu_allocate_irqs(trigger_signal, event, 1);
> +    qemu_irq irq = qemu_allocate_irq(trigger_signal, event, 0);
> +
> +    irq_cpu_hotplug = irq;
> +    qemu_free_irq(irq);

This looks... odd. But then, the whole code structure here with its
global variable and its roundabout way of triggering the notification
looks weird. We'd probably be better off looking up the sclp event for
cpu hotplug and triggering the interrupt directly.

>      return 0;
>  }
>
Peter Maydell May 28, 2015, 1:21 p.m. UTC | #2 
On 28 May 2015 at 13:08, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> @@ -81,7 +81,10 @@ static void trigger_signal(void *opaque, int n, int level)
>
>  static int irq_cpu_hotplug_init(SCLPEvent *event)
>  {
> -    irq_cpu_hotplug = qemu_allocate_irqs(trigger_signal, event, 1);
> +    qemu_irq irq = qemu_allocate_irq(trigger_signal, event, 0);
> +
> +    irq_cpu_hotplug = irq;
> +    qemu_free_irq(irq);
>      return 0;

Huh? Surely we can't validly use the irq once you've
called qemu_free_irq() on it?

-- PMM
Shannon Zhao May 30, 2015, 7:27 a.m. UTC | #3 
On 2015/5/28 21:11, Cornelia Huck wrote:
> On Thu, 28 May 2015 20:08:52 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> 
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > valgrind complains about:
>> > ==1413== 188 (8 direct, 180 indirect) bytes in 1 blocks are definitely lost in loss record 951 of 1,199
>> > ==1413==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>> > ==1413==    by 0x262D8B: malloc_and_trace (vl.c:2556)
>> > ==1413==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
>> > ==1413==    by 0x2C7ACF: qemu_extend_irqs (irq.c:55)
>> > ==1413==    by 0x2C7B5B: qemu_allocate_irqs (irq.c:64)
>> > ==1413==    by 0x2168F4: irq_cpu_hotplug_init (sclpcpu.c:84)
>> > ==1413==    by 0x21623F: event_realize (event-facility.c:385)
>> > ==1413==    by 0x2C4610: device_set_realized (qdev.c:1058)
>> > ==1413==    by 0x317DDA: property_set_bool (object.c:1514)
>> > ==1413==    by 0x3166D4: object_property_set (object.c:837)
>> > ==1413==    by 0x3189F6: object_property_set_qobject (qom-qobject.c:24)
>> > ==1413==    by 0x316943: object_property_set_bool (object.c:905)
>> > 
>> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> >  hw/s390x/sclpcpu.c | 9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
>> > index 2fe8b5a..1090e2f 100644
>> > --- a/hw/s390x/sclpcpu.c
>> > +++ b/hw/s390x/sclpcpu.c
>> > @@ -25,13 +25,13 @@ typedef struct ConfigMgtData {
>> >      uint8_t event_qualifier;
>> >  } QEMU_PACKED ConfigMgtData;
>> > 
>> > -static qemu_irq *irq_cpu_hotplug; /* Only used in this file */
>> > +static qemu_irq irq_cpu_hotplug; /* Only used in this file */
>> > 
>> >  #define EVENT_QUAL_CPU_CHANGE  1
>> > 
>> >  void raise_irq_cpu_hotplug(void)
>> >  {
>> > -    qemu_irq_raise(*irq_cpu_hotplug);
>> > +    qemu_irq_raise(irq_cpu_hotplug);
>> >  }
>> > 
>> >  static unsigned int send_mask(void)
>> > @@ -81,7 +81,10 @@ static void trigger_signal(void *opaque, int n, int level)
>> > 
>> >  static int irq_cpu_hotplug_init(SCLPEvent *event)
>> >  {
>> > -    irq_cpu_hotplug = qemu_allocate_irqs(trigger_signal, event, 1);
>> > +    qemu_irq irq = qemu_allocate_irq(trigger_signal, event, 0);
>> > +
>> > +    irq_cpu_hotplug = irq;
>> > +    qemu_free_irq(irq);
> This looks... odd. But then, the whole code structure here with its
> global variable and its roundabout way of triggering the notification
> looks weird. We'd probably be better off looking up the sclp event for
> cpu hotplug and triggering the interrupt directly.
> 

Yeah, it's odd and　I don't find who calls raise_irq_cpu_hotplug().
Paolo Bonzini May 30, 2015, 10:34 a.m. UTC | #4 
On 28/05/2015 14:08, Shannon Zhao wrote:
> -static qemu_irq *irq_cpu_hotplug; /* Only used in this file */
> +static qemu_irq irq_cpu_hotplug; /* Only used in this file */
>  
>  #define EVENT_QUAL_CPU_CHANGE  1
>  
>  void raise_irq_cpu_hotplug(void)
>  {
> -    qemu_irq_raise(*irq_cpu_hotplug);
> +    qemu_irq_raise(irq_cpu_hotplug);
>  }
>  
>  static unsigned int send_mask(void)
> @@ -81,7 +81,10 @@ static void trigger_signal(void *opaque, int n, int level)
>  
>  static int irq_cpu_hotplug_init(SCLPEvent *event)
>  {
> -    irq_cpu_hotplug = qemu_allocate_irqs(trigger_signal, event, 1);
> +    qemu_irq irq = qemu_allocate_irq(trigger_signal, event, 0);
> +
> +    irq_cpu_hotplug = irq;
> +    qemu_free_irq(irq);
>      return 0;

This is wrong, you cannot free the IRQ after you have stored it in
irq_cpu_hotplug.

Paolo
Shannon Zhao May 30, 2015, 11:07 a.m. UTC | #5 
On 2015/5/30 18:34, Paolo Bonzini wrote:
>
> On 28/05/2015 14:08, Shannon Zhao wrote:
>> >-static qemu_irq *irq_cpu_hotplug; /* Only used in this file */
>> >+static qemu_irq irq_cpu_hotplug; /* Only used in this file */
>> >
>> >  #define EVENT_QUAL_CPU_CHANGE  1
>> >
>> >  void raise_irq_cpu_hotplug(void)
>> >  {
>> >-    qemu_irq_raise(*irq_cpu_hotplug);
>> >+    qemu_irq_raise(irq_cpu_hotplug);
>> >  }
>> >
>> >  static unsigned int send_mask(void)
>> >@@ -81,7 +81,10 @@ static void trigger_signal(void *opaque, int n, int level)
>> >
>> >  static int irq_cpu_hotplug_init(SCLPEvent *event)
>> >  {
>> >-    irq_cpu_hotplug = qemu_allocate_irqs(trigger_signal, event, 1);
>> >+    qemu_irq irq = qemu_allocate_irq(trigger_signal, event, 0);
>> >+
>> >+    irq_cpu_hotplug = irq;
>> >+    qemu_free_irq(irq);
>> >      return 0;
> This is wrong, you cannot free the IRQ after you have stored it in
> irq_cpu_hotplug.

Yeah, sorry for that. But I don't find who calls 
raise_irq_cpu_hotplug(). I'm not very familiar with these codes.
diff mbox

Patch

diff --git a/hw/s390x/sclpcpu.c b/hw/s390x/sclpcpu.c
index 2fe8b5a..1090e2f 100644
--- a/hw/s390x/sclpcpu.c
+++ b/hw/s390x/sclpcpu.c
@@ -25,13 +25,13 @@  typedef struct ConfigMgtData {
     uint8_t event_qualifier;
 } QEMU_PACKED ConfigMgtData;
 
-static qemu_irq *irq_cpu_hotplug; /* Only used in this file */
+static qemu_irq irq_cpu_hotplug; /* Only used in this file */
 
 #define EVENT_QUAL_CPU_CHANGE  1
 
 void raise_irq_cpu_hotplug(void)
 {
-    qemu_irq_raise(*irq_cpu_hotplug);
+    qemu_irq_raise(irq_cpu_hotplug);
 }
 
 static unsigned int send_mask(void)
@@ -81,7 +81,10 @@  static void trigger_signal(void *opaque, int n, int level)
 
 static int irq_cpu_hotplug_init(SCLPEvent *event)
 {
-    irq_cpu_hotplug = qemu_allocate_irqs(trigger_signal, event, 1);
+    qemu_irq irq = qemu_allocate_irq(trigger_signal, event, 0);
+
+    irq_cpu_hotplug = irq;
+    qemu_free_irq(irq);
     return 0;
 }