Do not leave files behind in /tmp from testing
Commit Message
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.
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.
Comments
* 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>.
@@ -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)
{
@@ -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");
@@ -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[] =
@@ -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);
}
@@ -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);
}