From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54565) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxwT9-0005tW-KU for qemu-devel@nongnu.org; Thu, 28 May 2015 07:59:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxwT8-0007fv-Cs for qemu-devel@nongnu.org; Thu, 28 May 2015 07:59:27 -0400 Message-ID: <556702F8.50009@huawei.com> Date: Thu, 28 May 2015 19:58:48 +0800 From: Shannon Zhao MIME-Version: 1.0 References: <1432811625-13392-1-git-send-email-zhaoshenglong@huawei.com> <1432811625-13392-3-git-send-email-zhaoshenglong@huawei.com> <55670122.50101@msgid.tls.msk.ru> In-Reply-To: <55670122.50101@msgid.tls.msk.ru> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/4] hw/alpha/dp264.c: Fix memory leak spotted by valgrind List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev , qemu-devel@nongnu.org Cc: qemu-trivial@nongnu.org, pbonzini@redhat.com, shannon.zhao@linaro.org On 2015/5/28 19:50, Michael Tokarev wrote: > 28.05.2015 14:13, Shannon Zhao пишет: >> From: Shannon Zhao >> >> 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 >> Signed-off-by: Shannon Zhao >> --- >> 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