| Message ID | 1512063084-12105-1-git-send-email-ari@tuxera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 30, 2017 at 7:31 PM, Ari Sundholm <ari@tuxera.com> wrote: > This patch fixes the memory leaks we found when running fsstress under > valgrind. > > Signed-off-by: Ari Sundholm <ari@tuxera.com> > --- > ltp/fsstress.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > index 96f48b1..13d5dd5 100644 > --- a/ltp/fsstress.c > +++ b/ltp/fsstress.c > @@ -614,6 +614,9 @@ int main(int argc, char **argv) > return 1; > } > #endif > + > + cleanup_flist(); > + free(freq_table); > return 0; > } > } > @@ -640,6 +643,7 @@ int main(int argc, char **argv) > close(fd); > } > > + free(freq_table); > unlink(buf); > return 0; > } > @@ -997,6 +1001,7 @@ doproc(void) > } > errout: > chdir(".."); > + free(homedir); > if (cleanup) { > sprintf(cmd, "rm -rf %s", buf); > system(cmd); I don't see the point in those cleanups before returning from main() and I don't see the point in storing homedir in the first place at all and calling chdir(homedir) before abort. Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/03/2017 10:47 AM, Amir Goldstein wrote: > On Thu, Nov 30, 2017 at 7:31 PM, Ari Sundholm <ari@tuxera.com> wrote: >> This patch fixes the memory leaks we found when running fsstress under >> valgrind. >> >> Signed-off-by: Ari Sundholm <ari@tuxera.com> >> --- >> ltp/fsstress.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/ltp/fsstress.c b/ltp/fsstress.c >> index 96f48b1..13d5dd5 100644 >> --- a/ltp/fsstress.c >> +++ b/ltp/fsstress.c >> @@ -614,6 +614,9 @@ int main(int argc, char **argv) >> return 1; >> } >> #endif >> + >> + cleanup_flist(); >> + free(freq_table); >> return 0; >> } >> } >> @@ -640,6 +643,7 @@ int main(int argc, char **argv) >> close(fd); >> } >> >> + free(freq_table); >> unlink(buf); >> return 0; >> } >> @@ -997,6 +1001,7 @@ doproc(void) >> } >> errout: >> chdir(".."); >> + free(homedir); >> if (cleanup) { >> sprintf(cmd, "rm -rf %s", buf); >> system(cmd); > > > I don't see the point in those cleanups before returning from main() > and I don't see the point in storing homedir in the first place at all > and calling chdir(homedir) before abort. > I view cleanups like this simply a matter of correctness with regard to resource allocation/deallocation, even if we know such resources would be automatically freed by the OS on process termination. Also, from a practical POV, it is annoying to get complaints from valgrind and similar tools because of issues like this when investigating something else. I have no comment on the purpose of saving homedir. Best regards, Ari Sundholm ari@tuxera.com > Amir. > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 15, 2017 at 3:30 PM, Ari Sundholm <ari@tuxera.com> wrote: > On 12/03/2017 10:47 AM, Amir Goldstein wrote: >> >> On Thu, Nov 30, 2017 at 7:31 PM, Ari Sundholm <ari@tuxera.com> wrote: >>> >>> This patch fixes the memory leaks we found when running fsstress under >>> valgrind. >>> >>> Signed-off-by: Ari Sundholm <ari@tuxera.com> >>> --- >>> ltp/fsstress.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/ltp/fsstress.c b/ltp/fsstress.c >>> index 96f48b1..13d5dd5 100644 >>> --- a/ltp/fsstress.c >>> +++ b/ltp/fsstress.c >>> @@ -614,6 +614,9 @@ int main(int argc, char **argv) >>> return 1; >>> } >>> #endif >>> + >>> + cleanup_flist(); >>> + free(freq_table); >>> return 0; >>> } >>> } >>> @@ -640,6 +643,7 @@ int main(int argc, char **argv) >>> close(fd); >>> } >>> >>> + free(freq_table); >>> unlink(buf); >>> return 0; >>> } >>> @@ -997,6 +1001,7 @@ doproc(void) >>> } >>> errout: >>> chdir(".."); >>> + free(homedir); >>> if (cleanup) { >>> sprintf(cmd, "rm -rf %s", buf); >>> system(cmd); >> >> >> >> I don't see the point in those cleanups before returning from main() >> and I don't see the point in storing homedir in the first place at all >> and calling chdir(homedir) before abort. >> > > I view cleanups like this simply a matter of correctness with regard to > resource allocation/deallocation, even if we know such resources would be > automatically freed by the OS on process termination. Also, from a practical > POV, it is annoying to get complaints from valgrind and similar tools > because of issues like this when investigating something else. Fine, so at least change the change description. This is not a memory leak fix. This is a valgrind false positive noise reduction fix. Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" 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/ltp/fsstress.c b/ltp/fsstress.c index 96f48b1..13d5dd5 100644 --- a/ltp/fsstress.c +++ b/ltp/fsstress.c @@ -614,6 +614,9 @@ int main(int argc, char **argv) return 1; } #endif + + cleanup_flist(); + free(freq_table); return 0; } } @@ -640,6 +643,7 @@ int main(int argc, char **argv) close(fd); } + free(freq_table); unlink(buf); return 0; } @@ -997,6 +1001,7 @@ doproc(void) } errout: chdir(".."); + free(homedir); if (cleanup) { sprintf(cmd, "rm -rf %s", buf); system(cmd);
This patch fixes the memory leaks we found when running fsstress under valgrind. Signed-off-by: Ari Sundholm <ari@tuxera.com> --- ltp/fsstress.c | 5 +++++ 1 file changed, 5 insertions(+)