[Qemu-devel] [PATCH 0/4] Fix memory leak due to calling qemu_find_file and not freeing return buf

[Qemu-devel] [PATCH 0/4] Fix memory leak due to calling qemu_find_file and not freeing return buf

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Before I sent some patches to fix memory leak spotted by valgrind. Then
I'd like to dig deeper and find that two places have memory leak due to
calling qemu_find_file and not freeing return buf. Then through code
searching another two places are found. So this patchset is to fix them.

Shannon Zhao (4):
  hw/display/cg3.c: Fix memory leak
  hw/alpha/dp264.c: Fix memory leak spotted by valgrind
  hw/ppc/e500.c: Fix memory leak
  hw/display/tcx.c: Fix memory leak spotted by valgrind

 hw/alpha/dp264.c | 11 +++++++----
 hw/display/cg3.c |  1 +
 hw/display/tcx.c |  1 +
 hw/ppc/e500.c    |  2 ++
 4 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.0.4

[Qemu-devel] [PATCH 1/4] hw/display/cg3.c: Fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/display/cg3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 1e6ff2b..b86e5c0 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -302,6 +302,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
     if (fcode_filename) {
         ret = load_image_targphys(fcode_filename, s->prom_addr,
                                   FCODE_MAX_ROM_SIZE);
+        g_free(fcode_filename);
         if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
             error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
         }
-- 
2.0.4

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/4] hw/display/cg3.c: Fix memory leak

[Michael Tokarev]

28.05.2015 14:13, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/display/cg3.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index 1e6ff2b..b86e5c0 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -302,6 +302,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>      if (fcode_filename) {
>          ret = load_image_targphys(fcode_filename, s->prom_addr,
>                                    FCODE_MAX_ROM_SIZE);
> +        g_free(fcode_filename);
>          if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
>              error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
>          }

BTW, here, maybe it is better to rework error message too,
to include the actual filename it tried to load, not the
base name of it.

Is the error fatal?

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/4] hw/display/cg3.c: Fix memory leak

[Shannon Zhao]

On 2015/5/28 19:55, Michael Tokarev wrote:
> 28.05.2015 14:13, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> >  hw/display/cg3.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> > 
>> > diff --git a/hw/display/cg3.c b/hw/display/cg3.c
>> > index 1e6ff2b..b86e5c0 100644
>> > --- a/hw/display/cg3.c
>> > +++ b/hw/display/cg3.c
>> > @@ -302,6 +302,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>> >      if (fcode_filename) {
>> >          ret = load_image_targphys(fcode_filename, s->prom_addr,
>> >                                    FCODE_MAX_ROM_SIZE);
>> > +        g_free(fcode_filename);
>> >          if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
>> >              error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
>> >          }
> BTW, here, maybe it is better to rework error message too,
> to include the actual filename it tried to load, not the
> base name of it.
> 

So respin this and other patches?

> Is the error fatal?

If CG3_ROM_FILE is removed, it happens.

-- 
Shannon

[Qemu-devel] [PATCH 2/4] hw/alpha/dp264.c: Fix memory leak spotted by valgrind

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

valgrind complains about:
==7055== 58 bytes in 1 blocks are definitely lost in loss record 1,471 of 2,192
==7055==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7055==    by 0x24410F: malloc_and_trace (vl.c:2556)
==7055==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
==7055==    by 0x64DEFD7: g_strndup (in /usr/lib64/libglib-2.0.so.0.3600.3)
==7055==    by 0x650181A: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
==7055==    by 0x64DF0CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
==7055==    by 0x64DF188: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.3600.3)
==7055==    by 0x242F81: qemu_find_file (vl.c:2121)
==7055==    by 0x217A32: clipper_init (dp264.c:105)
==7055==    by 0x2484DA: main (vl.c:4249)

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/alpha/dp264.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 9fe7e8b..0c5395f 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -56,6 +56,7 @@ static void clipper_init(MachineState *machine)
     qemu_irq rtc_irq;
     long size, i;
     const char *palcode_filename;
+    char *filename;
     uint64_t palcode_entry, palcode_low, palcode_high;
     uint64_t kernel_entry, kernel_low, kernel_high;
 
@@ -102,18 +103,20 @@ static void clipper_init(MachineState *machine)
        but one explicitly written for the emulation, we might as
        well load it directly from and ELF image.  */
     palcode_filename = (bios_name ? bios_name : "palcode-clipper");
-    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
-    if (palcode_filename == NULL) {
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
+    if (filename == NULL) {
         hw_error("no palcode provided\n");
         exit(1);
     }
-    size = load_elf(palcode_filename, cpu_alpha_superpage_to_phys,
+    size = load_elf(filename, cpu_alpha_superpage_to_phys,
                     NULL, &palcode_entry, &palcode_low, &palcode_high,
                     0, EM_ALPHA, 0);
     if (size < 0) {
-        hw_error("could not load palcode '%s'\n", palcode_filename);
+        hw_error("could not load palcode '%s'\n", filename);
+        g_free(filename);
         exit(1);
     }
+    g_free(filename);
 
     /* Start all cpus at the PALcode RESET entry point.  */
     for (i = 0; i < smp_cpus; ++i) {
-- 
2.0.4

Re: [Qemu-devel] [PATCH 2/4] hw/alpha/dp264.c: Fix memory leak spotted by valgrind

[Michael Tokarev]

28.05.2015 14:13, Shannon Zhao пишет:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> valgrind complains about:
> ==7055== 58 bytes in 1 blocks are definitely lost in loss record 1,471 of 2,192
> ==7055==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==7055==    by 0x24410F: malloc_and_trace (vl.c:2556)
> ==7055==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==7055==    by 0x64DEFD7: g_strndup (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==7055==    by 0x650181A: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==7055==    by 0x64DF0CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==7055==    by 0x64DF188: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.3600.3)
> ==7055==    by 0x242F81: qemu_find_file (vl.c:2121)
> ==7055==    by 0x217A32: clipper_init (dp264.c:105)
> ==7055==    by 0x2484DA: main (vl.c:4249)
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/alpha/dp264.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 9fe7e8b..0c5395f 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -56,6 +56,7 @@ static void clipper_init(MachineState *machine)
>      qemu_irq rtc_irq;
>      long size, i;
>      const char *palcode_filename;
> +    char *filename;
>      uint64_t palcode_entry, palcode_low, palcode_high;
>      uint64_t kernel_entry, kernel_low, kernel_high;
>  
> @@ -102,18 +103,20 @@ static void clipper_init(MachineState *machine)
>         but one explicitly written for the emulation, we might as
>         well load it directly from and ELF image.  */
>      palcode_filename = (bios_name ? bios_name : "palcode-clipper");
> -    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
> -    if (palcode_filename == NULL) {
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
> +    if (filename == NULL) {
>          hw_error("no palcode provided\n");
>          exit(1);
>      }
> -    size = load_elf(palcode_filename, cpu_alpha_superpage_to_phys,
> +    size = load_elf(filename, cpu_alpha_superpage_to_phys,
>                      NULL, &palcode_entry, &palcode_low, &palcode_high,
>                      0, EM_ALPHA, 0);
>      if (size < 0) {
> -        hw_error("could not load palcode '%s'\n", palcode_filename);
> +        hw_error("could not load palcode '%s'\n", filename);
> +        g_free(filename);

In a previous patch you _removed_ g_free() before exit(), here' you're
adding one.  I don't think there's any need to free things before exit(),
but at least we should be consistent, maybe at least within one series :)

>          exit(1);
>      }
> +    g_free(filename);

How about this:

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 9fe7e8b..f86e7bb 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -55,7 +55,7 @@ static void clipper_init(MachineState *machine)
     ISABus *isa_bus;
     qemu_irq rtc_irq;
     long size, i;
-    const char *palcode_filename;
+    char *palcode_filename;
     uint64_t palcode_entry, palcode_low, palcode_high;
     uint64_t kernel_entry, kernel_low, kernel_high;

@@ -101,8 +101,8 @@ static void clipper_init(MachineState *machine)
     /* Load PALcode.  Given that this is not "real" cpu palcode,
        but one explicitly written for the emulation, we might as
        well load it directly from and ELF image.  */
-    palcode_filename = (bios_name ? bios_name : "palcode-clipper");
-    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
+    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
+                                bios_name ? bios_name : "palcode-clipper");
     if (palcode_filename == NULL) {
         hw_error("no palcode provided\n");
         exit(1);
@@ -114,6 +114,7 @@ static void clipper_init(MachineState *machine)
         hw_error("could not load palcode '%s'\n", palcode_filename);
         exit(1);
     }
+    g_free(palcode_filename);

     /* Start all cpus at the PALcode RESET entry point.  */
     for (i = 0; i < smp_cpus; ++i) {


I'm not saying it is better, it is just that we don't really
need the original basename of the file and hence don't need
second variable.

If you like it I'd apply it, of I'll apply your version
(without g_free() before exit).

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 2/4] hw/alpha/dp264.c: Fix memory leak spotted by valgrind

[Shannon Zhao]

On 2015/5/28 19:50, Michael Tokarev wrote:
> 28.05.2015 14:13, Shannon Zhao пишет:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> valgrind complains about:
>> ==7055== 58 bytes in 1 blocks are definitely lost in loss record 1,471 of 2,192
>> ==7055==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==7055==    by 0x24410F: malloc_and_trace (vl.c:2556)
>> ==7055==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
>> ==7055==    by 0x64DEFD7: g_strndup (in /usr/lib64/libglib-2.0.so.0.3600.3)
>> ==7055==    by 0x650181A: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
>> ==7055==    by 0x64DF0CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
>> ==7055==    by 0x64DF188: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.3600.3)
>> ==7055==    by 0x242F81: qemu_find_file (vl.c:2121)
>> ==7055==    by 0x217A32: clipper_init (dp264.c:105)
>> ==7055==    by 0x2484DA: main (vl.c:4249)
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/alpha/dp264.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
>> index 9fe7e8b..0c5395f 100644
>> --- a/hw/alpha/dp264.c
>> +++ b/hw/alpha/dp264.c
>> @@ -56,6 +56,7 @@ static void clipper_init(MachineState *machine)
>>      qemu_irq rtc_irq;
>>      long size, i;
>>      const char *palcode_filename;
>> +    char *filename;
>>      uint64_t palcode_entry, palcode_low, palcode_high;
>>      uint64_t kernel_entry, kernel_low, kernel_high;
>>  
>> @@ -102,18 +103,20 @@ static void clipper_init(MachineState *machine)
>>         but one explicitly written for the emulation, we might as
>>         well load it directly from and ELF image.  */
>>      palcode_filename = (bios_name ? bios_name : "palcode-clipper");
>> -    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
>> -    if (palcode_filename == NULL) {
>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
>> +    if (filename == NULL) {
>>          hw_error("no palcode provided\n");
>>          exit(1);
>>      }
>> -    size = load_elf(palcode_filename, cpu_alpha_superpage_to_phys,
>> +    size = load_elf(filename, cpu_alpha_superpage_to_phys,
>>                      NULL, &palcode_entry, &palcode_low, &palcode_high,
>>                      0, EM_ALPHA, 0);
>>      if (size < 0) {
>> -        hw_error("could not load palcode '%s'\n", palcode_filename);
>> +        hw_error("could not load palcode '%s'\n", filename);
>> +        g_free(filename);
> 
> In a previous patch you _removed_ g_free() before exit(), here' you're
> adding one.  I don't think there's any need to free things before exit(),
> but at least we should be consistent, maybe at least within one series :)
> 
>>          exit(1);
>>      }
>> +    g_free(filename);
> 
> How about this:
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 9fe7e8b..f86e7bb 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -55,7 +55,7 @@ static void clipper_init(MachineState *machine)
>      ISABus *isa_bus;
>      qemu_irq rtc_irq;
>      long size, i;
> -    const char *palcode_filename;
> +    char *palcode_filename;
>      uint64_t palcode_entry, palcode_low, palcode_high;
>      uint64_t kernel_entry, kernel_low, kernel_high;
> 
> @@ -101,8 +101,8 @@ static void clipper_init(MachineState *machine)
>      /* Load PALcode.  Given that this is not "real" cpu palcode,
>         but one explicitly written for the emulation, we might as
>         well load it directly from and ELF image.  */
> -    palcode_filename = (bios_name ? bios_name : "palcode-clipper");
> -    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, palcode_filename);
> +    palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
> +                                bios_name ? bios_name : "palcode-clipper");
>      if (palcode_filename == NULL) {
>          hw_error("no palcode provided\n");
>          exit(1);
> @@ -114,6 +114,7 @@ static void clipper_init(MachineState *machine)
>          hw_error("could not load palcode '%s'\n", palcode_filename);
>          exit(1);
>      }
> +    g_free(palcode_filename);
> 
>      /* Start all cpus at the PALcode RESET entry point.  */
>      for (i = 0; i < smp_cpus; ++i) {
> 
> 
> I'm not saying it is better, it is just that we don't really
> need the original basename of the file and hence don't need
> second variable.
> 
Yes, I've thought about this way but it's gone.

> If you like it I'd apply it, of I'll apply your version
> (without g_free() before exit).
> 
I'm good with your version.

Thanks,
-- 
Shannon

[Qemu-devel] [PATCH 3/4] hw/ppc/e500.c: Fix memory leak

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/ppc/e500.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c10e1b5..f74e6f2 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1027,9 +1027,11 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
                                   NULL, NULL);
         if (kernel_size < 0) {
             fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
+            g_free(filename);
             exit(1);
         }
     }
+    g_free(filename);
 
     /* Reserve space for dtb */
     dt_base = (loadaddr + bios_size + DTC_LOAD_PAD) & ~DTC_PAD_MASK;
-- 
2.0.4

Re: [Qemu-devel] [PATCH 3/4] hw/ppc/e500.c: Fix memory leak

[Michael Tokarev]

28.05.2015 14:13, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/ppc/e500.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index c10e1b5..f74e6f2 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -1027,9 +1027,11 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>                                    NULL, NULL);
>          if (kernel_size < 0) {
>              fprintf(stderr, "qemu: could not load firmware '%s'\n", filename);
> +            g_free(filename);
>              exit(1);
>          }
>      }
> +    g_free(filename);

Hm.  This is probably the patch I was thinking about when saying
you _removed" g_free() before exit(), but you're _adding_ one… ;)
My bad.

Anyway, I don't think there's any reason to add such free() before
exiting.  Second g_free() is okay, first is unnecessary.  I think.

Thanks,

/mjt

[Qemu-devel] [PATCH 4/4] hw/display/tcx.c: Fix memory leak spotted by valgrind

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

valgrind complains about:
==23693== 55 bytes in 1 blocks are definitely lost in loss record 1,277 of 2,014
==23693==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23693==    by 0x21B93F: malloc_and_trace (vl.c:2556)
==23693==    by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
==23693==    by 0x64DEFD7: g_strndup (in /usr/lib64/libglib-2.0.so.0.3600.3)
==23693==    by 0x650181A: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
==23693==    by 0x64DF0CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.3600.3)
==23693==    by 0x64DF188: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.3600.3)
==23693==    by 0x21A7B1: qemu_find_file (vl.c:2121)
==23693==    by 0x1E4F6E: tcx_realizefn (tcx.c:1013)
==23693==    by 0x26CC20: device_set_realized (qdev.c:1058)
==23693==    by 0x2B6766: property_set_bool (object.c:1514)
==23693==    by 0x2B5060: object_property_set (object.c:837)

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/display/tcx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index a9f9f66..6c5e584 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -1014,6 +1014,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     if (fcode_filename) {
         ret = load_image_targphys(fcode_filename, s->prom_addr,
                                   FCODE_MAX_ROM_SIZE);
+        g_free(fcode_filename);
         if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
             error_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
         }
-- 
2.0.4

Re: [Qemu-devel] [PATCH 4/4] hw/display/tcx.c: Fix memory leak spotted by valgrind

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH 4/4] hw/display/tcx.c: Fix memory leak spotted by valgrind

[Michael Tokarev]

28.05.2015 14:54, Michael Tokarev wrote:
> Applied to -trivial, thanks!

..or not applied yet... :)

This is again the same situation with the error message as with
1/4 hw/display/cg3.c, and the same question - is the error fatal?

And note that you're the number-one person who made changes to
this file... ;)

/mjt

Re: [Qemu-devel] [PATCH 4/4] hw/display/tcx.c: Fix memory leak spotted by valgrind

[Mark Cave-Ayland]

On 28/05/15 13:01, Michael Tokarev wrote:

> 28.05.2015 14:54, Michael Tokarev wrote:
>> Applied to -trivial, thanks!
> 
> ..or not applied yet... :)
> 
> This is again the same situation with the error message as with
> 1/4 hw/display/cg3.c, and the same question - is the error fatal?
> 
> And note that you're the number-one person who made changes to
> this file... ;)
> 
> /mjt

The FCode ROMs supplied for CG3/TCX are used to both initialise various
DT entries for the graphic adapters and also provide a minimal driver to
allow OpenBIOS (or Sun PROM) to initialise and use the adapter at boot.

I'd say at the moment the error should be fatal, since the correct way
to have a headless system would be not to have the virtual graphics
adapter plugged into the system in the first place. Otherwise I can see
that the skeleton DT nodes generated by the PROM would be missing
several properties generated by the FCode (such as the framebuffer
address), which would be utterly confusing for any client trying to use
the graphics adapter.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 4/4] hw/display/tcx.c: Fix memory leak spotted by valgrind

[Michael Tokarev]

28.05.2015 15:19, Mark Cave-Ayland wrote:
[]
> The FCode ROMs supplied for CG3/TCX are used to both initialise various
> DT entries for the graphic adapters and also provide a minimal driver to
> allow OpenBIOS (or Sun PROM) to initialise and use the adapter at boot.
> 
> I'd say at the moment the error should be fatal, [...]

So it looks like some exit(1) or similar is missing after error_report()
in these cases, which should be added :)  Thanks!

/mjt