| Message ID | 1373791722-29998-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
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…… Thanks, Wang > From: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > > After calling opendir() successfully, closedir() should be > also called to free memory. Otherwise, memory leak happens. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > Reviewed-by: Anand Jain <anand.jain@oracle.com> > --- > 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
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 <wangsl.fnst@cn.fujitsu.com> >> >> After calling opendir() successfully, closedir() should be >> also called to free memory. Otherwise, memory leak happens. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >> Reviewed-by: Anand Jain <anand.jain@oracle.com> >> --- >> 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 > -- 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
> > > 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. Yeah, i try to do something like: open_file_or_dir(char *name, DIR **dirstream) It really touches a lot of places.. So i hesitate if there is a better method to do this.. Thanks Wang > > Thanks, Anand > > >> Thanks, >> Wang >>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >>> >>> After calling opendir() successfully, closedir() should be >>> also called to free memory. Otherwise, memory leak happens. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >>> Reviewed-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> 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 >> -- 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
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; }