| Message ID | alpine.DEB.2.10.1510152049430.6002@digraph.polyomino.org.uk |
|---|---|
| State | New |
| Headers | show |
* Joseph Myers: > I noticed that glibc testsuite runs left several files behind in /tmp > (or TMPDIR, if different). The problem was testcases that generate a > template for mkstemp / mkstemp64, ending with XXXXXX, then pass that > template to add_temp_file before calling mkstemp / mkstemp64, meaning > that the template ending with XXXXXX is stored in the list of > temporary files to delete (add_temp_file uses strdup so that the > original string doesn't need to stay live), not the actual filename as > determined by mkstemp / mkstemp64. This patch fixes those tests to > call add_temp_file later. Oops, I think I broken this while fixing the memory leak in the test driver. > Tested for x86_64 (that the files are no longer left behind by a > testsuite run and the modified tests still pass). > > 2015-10-15 Joseph Myers <joseph@codesourcery.com> > > * io/test-lfs.c (do_prepare): Do not call add_temp_file until > after mkstemp64. > * io/tst-fcntl.c (do_prepare): Do not call add_temp_file here. > (do_test): Call add_temp_file here instead. > * login/tst-utmp.c (do_prepare): Do not call add_temp_file until > after mkstemp. > * rt/tst-aio.c (do_prepare): Likewise. > * rt/tst-aio64.c (do_prepare): Likewise. Okay. Thanks, Florian
> * io/tst-fcntl.c (do_prepare): Do not call add_temp_file here. > (do_test): Call add_temp_file here instead. This is a regression in the TEST_DIRECT case. Is there a reason you can't move the mkstemp call into do_prepare as is done in other test cases?
On Thu, 15 Oct 2015, Roland McGrath wrote: > > * io/tst-fcntl.c (do_prepare): Do not call add_temp_file here. > > (do_test): Call add_temp_file here instead. > > This is a regression in the TEST_DIRECT case. Is there a reason you can't > move the mkstemp call into do_prepare as is done in other test cases? I don't see why this should be a regression - the only effect should be that a file ending with XXXXXX (that doesn't exist) doesn't get added to the list of temporary files, but a name that does exist gets added after creation. There's no point calling add_temp_file on a file that doesn't and won't exist. I don't know if the test has a reason for not calling mkstemp until after fork. As I read the code, temporary files will be deleted in both the parent and the child, as the list is preserved across fork. If anything, that would make it cleaner to do all the file creation after fork (in do_test not do_prepare) so that files are only deleted once, rather than both parent and child trying to delete the same file in the case of files created before fork.
What I meant was that it's a regression in the parity of behavior between normal and TEST_DIRECT cases. In the TEST_DIRECT case, there is no fork. The only way that files get cleaned up is if they are in entered into the temp_name_list in the PREPARE phase.
On Thu, 15 Oct 2015, Roland McGrath wrote: > What I meant was that it's a regression in the parity of behavior between > normal and TEST_DIRECT cases. In the TEST_DIRECT case, there is no fork. > The only way that files get cleaned up is if they are in entered into the > temp_name_list in the PREPARE phase. I don't see why that would be the case. delete_temp_files is called (only) via atexit - that applies whether or not there's a fork, and delete_temp_files should be called whether exit it called or the program returns from main (the case of calling _exit, and so not calling atexit handlers, doesn't apply to this test). It shouldn't matter at all which phase the files get entered into temp_name_list in.
> On Thu, 15 Oct 2015, Roland McGrath wrote: > > > What I meant was that it's a regression in the parity of behavior between > > normal and TEST_DIRECT cases. In the TEST_DIRECT case, there is no fork. > > The only way that files get cleaned up is if they are in entered into the > > temp_name_list in the PREPARE phase. > > I don't see why that would be the case. Then apparently you have not noticed the point of the TEST_DIRECT case. > delete_temp_files is called (only) via atexit - that applies whether or > not there's a fork, and delete_temp_files should be called whether exit > it called or the program returns from main (the case of calling _exit, > and so not calling atexit handlers, doesn't apply to this test). It > shouldn't matter at all which phase the files get entered into > temp_name_list in. If the test exits abnormally, then it won't call delete_temp_files at all. In the normal case, the parent is there to catch the dying child and do appropriate cleanup, so it matters that the parent's delete_temp_files cover everything that the child's would have. (I acknowledge that this matters less if it's not an EXPECTED_SIGNAL case, since /tmp turds left on failures are less of a concern.) The TEST_DIRECT case is similar in effect: the cleanup on abnormal exit is handled by the wrapper script, which needs to be instructed between PREPARE and TEST_FUNCTION.
On Thu, 15 Oct 2015, Roland McGrath wrote: > If the test exits abnormally, then it won't call delete_temp_files at all. > In the normal case, the parent is there to catch the dying child and do > appropriate cleanup, so it matters that the parent's delete_temp_files > cover everything that the child's would have. (I acknowledge that this > matters less if it's not an EXPECTED_SIGNAL case, since /tmp turds left on > failures are less of a concern.) The TEST_DIRECT case is similar in > effect: the cleanup on abnormal exit is handled by the wrapper script, > which needs to be instructed between PREPARE and TEST_FUNCTION. I still don't see what this has to do with my patch. It may be a case for a general review of where tests create and register temporary files to be entered on the wiki todo list. But the add_temp_file call that my patch moves could never have done anything useful before (because it registered the wrong name) - so I don't see how my patch could regress anything that worked before (and in the case of normal termination, my patch should fix things whether or not TEST_DIRECT applies).
> I still don't see what this has to do with my patch. It may be a case for > a general review of where tests create and register temporary files to be > entered on the wiki todo list. But the add_temp_file call that my patch > moves could never have done anything useful before (because it registered > the wrong name) - so I don't see how my patch could regress anything that > worked before (and in the case of normal termination, my patch should fix > things whether or not TEST_DIRECT applies). Your patch purports to be a cleanup, but for this case it changes things in the wrong direction. Since there is no controversy about the other changes in your patch, perhaps you should commit those separately so they are not conflated with the more complex issues raised by this one change.
On Thu, 15 Oct 2015, Roland McGrath wrote: > Since there is no controversy about the other changes in your patch, > perhaps you should commit those separately so they are not conflated > with the more complex issues raised by this one change. I've done that, and added the general issue of reviewing add_temp_file / create_temp_file uses (of which there are many outside the preparation stage of tests) to <https://sourceware.org/glibc/wiki/Development_Todo/Master#Use_test-skeleton.c>.
diff --git a/io/test-lfs.c b/io/test-lfs.c index b6ebae4..09310a4 100644 --- a/io/test-lfs.c +++ b/io/test-lfs.c @@ -56,7 +56,6 @@ do_prepare (int argc, char *argv[]) name = malloc (name_len + sizeof ("/lfsXXXXXX")); mempcpy (mempcpy (name, test_dir, name_len), "/lfsXXXXXX", sizeof ("/lfsXXXXXX")); - add_temp_file (name); /* Open our test file. */ fd = mkstemp64 (name); @@ -71,6 +70,7 @@ do_prepare (int argc, char *argv[]) else error (EXIT_FAILURE, errno, "cannot create temporary file"); } + add_temp_file (name); if (getrlimit64 (RLIMIT_FSIZE, &rlim) != 0) { diff --git a/io/tst-fcntl.c b/io/tst-fcntl.c index dfdb42e..c7b8275 100644 --- a/io/tst-fcntl.c +++ b/io/tst-fcntl.c @@ -47,7 +47,6 @@ do_prepare (int argc, char *argv[]) name = malloc (name_len + sizeof ("/fcntlXXXXXX")); mempcpy (mempcpy (name, test_dir, name_len), "/fcntlXXXXXX", sizeof ("/fcntlXXXXXX")); - add_temp_file (name); } @@ -68,6 +67,7 @@ do_test (int argc, char *argv[]) printf ("cannot open temporary file: %m\n"); return 1; } + add_temp_file (name); if (fstat64 (fd, &st) != 0) { printf ("cannot stat test file: %m\n"); diff --git a/login/tst-utmp.c b/login/tst-utmp.c index a69a556..a029003 100644 --- a/login/tst-utmp.c +++ b/login/tst-utmp.c @@ -65,12 +65,12 @@ do_prepare (int argc, char *argv[]) name = malloc (name_len + sizeof ("/utmpXXXXXX")); mempcpy (mempcpy (name, test_dir, name_len), "/utmpXXXXXX", sizeof ("/utmpXXXXXX")); - add_temp_file (name); /* Open our test file. */ fd = mkstemp (name); if (fd == -1) error (EXIT_FAILURE, errno, "cannot open test file `%s'", name); + add_temp_file (name); } struct utmp entry[] = diff --git a/rt/tst-aio.c b/rt/tst-aio.c index 783907e..007ef6c 100644 --- a/rt/tst-aio.c +++ b/rt/tst-aio.c @@ -54,12 +54,12 @@ do_prepare (int argc, char *argv[]) name = malloc (name_len + sizeof ("/aioXXXXXX")); mempcpy (mempcpy (name, test_dir, name_len), "/aioXXXXXX", sizeof ("/aioXXXXXX")); - add_temp_file (name); /* Open our test file. */ fd = mkstemp (name); if (fd == -1) error (EXIT_FAILURE, errno, "cannot open test file `%s'", name); + add_temp_file (name); } diff --git a/rt/tst-aio64.c b/rt/tst-aio64.c index 3a9ae79..b315eec 100644 --- a/rt/tst-aio64.c +++ b/rt/tst-aio64.c @@ -55,12 +55,12 @@ do_prepare (int argc, char *argv[]) name = malloc (name_len + sizeof ("/aioXXXXXX")); mempcpy (mempcpy (name, test_dir, name_len), "/aioXXXXXX", sizeof ("/aioXXXXXX")); - add_temp_file (name); /* Open our test file. */ fd = mkstemp (name); if (fd == -1) error (EXIT_FAILURE, errno, "cannot open test file `%s'", name); + add_temp_file (name); }