From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp1040.oracle.com ([141.146.126.69]:40919 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752536Ab3GNOlF (ORCPT ); Sun, 14 Jul 2013 10:41:05 -0400 Message-ID: <51E2B871.5000801@oracle.com> Date: Sun, 14 Jul 2013 22:40:49 +0800 From: anand jain MIME-Version: 1.0 To: Wang Shilong CC: "linux-btrfs@vger.kernel.org Btrfs" , David Sterba , "wangsl.fnst@cn.fujitsu.com" Subject: Re: [PATCH v2] Btrfs-progs: fix memory leak in open_file_or_dir() References: <1373410908-2274-1-git-send-email-wangshilong1991@gmail.com> <1373791722-29998-1-git-send-email-anand.jain@oracle.com> <4C2AE033-0C93-419A-9FD2-C8E0B0A7A942@gmail.com> In-Reply-To: <4C2AE033-0C93-419A-9FD2-C8E0B0A7A942@gmail.com> Content-Type: text/plain; charset=GB2312 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 14/07/2013 21:58, Wang Shilong wrote: > Hello Anand and David, > > I use the tool valgrind to whether there exists memory leak: > > valgrind --tool=memcheck --trace-origins=yes --leak-chek=full btrfs sub list /mnt > There still exists memory leak related to open_file_or_dir() (After applying this patch). > > I guess it is because we should call closedir(dirstram) rather than close(fd) if opening a directory. > Maybe we can make open_file_or_dir() something like: > open_file_or_dir(char *name, DIR **dirstream) > > if we opened a directory, we close it by calling closeidr(dirstream). > elsewise, close(fd) is ok!.. > > I am wondering why close(fd) can not just finish this workˇ­ˇ­ now I get your broader point of view. For some reason it gave me an impression you are trying to handle a proper cleanup operation if when dirfd(dirstream) fails and below patch would just address that. Now when open_file_or_dir() is successful we still need a proper closing code. May be its a good idea to write a new patch to include that.. it touches a lot of places. Thanks, Anand > Thanks, > Wang >> From: Wang Shilong >> >> After calling opendir() successfully, closedir() should be >> also called to free memory. Otherwise, memory leak happens. >> >> Signed-off-by: Anand Jain >> Signed-off-by: Wang Shilong >> Reviewed-by: Anand Jain >> --- >> utils.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/utils.c b/utils.c >> index d3bec9b..4b3c778 100644 >> --- a/utils.c >> +++ b/utils.c >> @@ -1503,7 +1503,7 @@ int open_file_or_dir(const char *fname) >> { >> int ret; >> struct stat st; >> - DIR *dirstream; >> + DIR *dirstream = NULL; >> int fd; >> >> ret = stat(fname, &st); >> @@ -1520,7 +1520,9 @@ int open_file_or_dir(const char *fname) >> fd = open(fname, O_RDWR); >> } >> if (fd < 0) { >> - return -3; >> + fd = -3; >> + if (dirstream) >> + closedir(dirstream); >> } >> return fd; >> } >> -- >> 1.7.7.6 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >