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 <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.
> 
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