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 <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Shannon Zhao <address@hidden>
> ---
>  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