[LTP] [PATCH] syscalls/mmap16: close open files in cleanup path

[LTP] [PATCH] syscalls/mmap16: close open files in cleanup path

[Peter Maydell]

If the mmap16 test fails while the do_test() function
still has its filedescriptor open, the cleanup function's
attempt to unmount will fail with EBUSY, resulting in a
lot of noise in the test log, a leaked mounted filesystem
and unnecessary test failures later in the run.

Make the do_test() file descriptor global so we can
close it in the cleanup function if necessary.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I noticed this because QEMU linux-user mode happens to fail
this test at the moment, so it exercises the broken failure path.

 testcases/kernel/syscalls/mmap/mmap16.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/mmap/mmap16.c b/testcases/kernel/syscalls/mmap/mmap16.c
index ca1f47d..5d943a0 100644
--- a/testcases/kernel/syscalls/mmap/mmap16.c
+++ b/testcases/kernel/syscalls/mmap/mmap16.c
@@ -48,6 +48,7 @@ static const char *device;
 static const char *fs_type = "ext4";
 static int mount_flag;
 static int chdir_flag;
+static int parentfd;
 
 static int page_size;
 static int bug_reproduced;
@@ -91,7 +92,7 @@ int main(int ac, char **av)
 
 static void do_test(void)
 {
-	int fd, ret, status;
+	int ret, status;
 	pid_t child;
 	char buf[FS_BLOCKSIZE];
 
@@ -105,12 +106,12 @@ static void do_test(void)
 	case 0:
 		do_child();
 	default:
-		fd = SAFE_OPEN(cleanup, "testfilep", O_RDWR);
+		parentfd = SAFE_OPEN(cleanup, "testfilep", O_RDWR);
 		memset(buf, 'a', FS_BLOCKSIZE);
 
 		TST_SAFE_CHECKPOINT_WAIT(cleanup, 0);
 		while (1) {
-			ret = write(fd, buf, FS_BLOCKSIZE);
+			ret = write(parentfd, buf, FS_BLOCKSIZE);
 			if (ret < 0) {
 				if (errno == ENOSPC) {
 					break;
@@ -120,7 +121,7 @@ static void do_test(void)
 				}
 			}
 		}
-		SAFE_CLOSE(cleanup, fd);
+		SAFE_CLOSE(cleanup, parentfd);
 		TST_SAFE_CHECKPOINT_WAKE(cleanup, 0);
 	}
 
@@ -231,6 +232,8 @@ static void do_child(void)
 
 static void cleanup(void)
 {
+	if (parentfd > 0)
+		close(parentfd);
 	if (chdir_flag && chdir(".."))
 		tst_resm(TWARN | TERRNO, "chdir('..') failed");
 	if (mount_flag && tst_umount(MNTPOINT) < 0)
-- 
1.9.1

[LTP] [PATCH] syscalls/mmap16: close open files in cleanup path

[Jan Stancek]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: ltp@lists.linux.it
> Cc: patches@linaro.org
> Sent: Friday, 1 July, 2016 4:55:25 PM
> Subject: [LTP] [PATCH] syscalls/mmap16: close open files in cleanup path
> 
> If the mmap16 test fails while the do_test() function
> still has its filedescriptor open, the cleanup function's
> attempt to unmount will fail with EBUSY, resulting in a
> lot of noise in the test log, a leaked mounted filesystem
> and unnecessary test failures later in the run.
> 
> Make the do_test() file descriptor global so we can
> close it in the cleanup function if necessary.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi,

pushed with small change, which treats uninitialized fd as -1, not 0.

Regards,
Jan

[LTP] [PATCH] syscalls/mmap16: close open files in cleanup path

[Peter Maydell]

On 11 July 2016 at 13:19, Jan Stancek <jstancek@redhat.com> wrote:
> From: "Peter Maydell" <peter.maydell@linaro.org>

>> If the mmap16 test fails while the do_test() function
>> still has its filedescriptor open, the cleanup function's
>> attempt to unmount will fail with EBUSY, resulting in a
>> lot of noise in the test log, a leaked mounted filesystem
>> and unnecessary test failures later in the run.

> pushed with small change, which treats uninitialized fd as -1, not 0.

Thanks. Could you explain why you added

+               parentfd = -1;

after the SAFE_CLOSE() line? doc/test-writing-guidelines.txt
says "The SAFE_CLOSE() function also sets the passed file
descriptor to -1 after it's successfully closed.", so if
that's not correct we should fix the docs.

That doc also gives an example (in section 2.2.1) that
says "Since global variables are initialized to zero we can
just check that fd > 0 before we attempt to close it.",
which is why I used 0 rather than -1. If current preferred
test style has changed it would be nice to update the
example code.

thanks
-- PMM

[LTP] [PATCH] syscalls/mmap16: close open files in cleanup path

[Jan Stancek]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it, "Patch Tracking" <patches@linaro.org>
> Sent: Monday, 11 July, 2016 3:18:02 PM
> Subject: Re: [LTP] [PATCH] syscalls/mmap16: close open files in cleanup path
> 
> On 11 July 2016 at 13:19, Jan Stancek <jstancek@redhat.com> wrote:
> > From: "Peter Maydell" <peter.maydell@linaro.org>
> 
> >> If the mmap16 test fails while the do_test() function
> >> still has its filedescriptor open, the cleanup function's
> >> attempt to unmount will fail with EBUSY, resulting in a
> >> lot of noise in the test log, a leaked mounted filesystem
> >> and unnecessary test failures later in the run.
> 
> > pushed with small change, which treats uninitialized fd as -1, not 0.
> 
> Thanks. Could you explain why you added
> 
> +               parentfd = -1;
> 
> after the SAFE_CLOSE() line? doc/test-writing-guidelines.txt
> says "The SAFE_CLOSE() function also sets the passed file
> descriptor to -1 after it's successfully closed.", so if
> that's not correct we should fix the docs.

It's true for newlib, but not for oldlib. I think we should
rather fix oldlib SAFE_CLOSE to match the docs, so I dropped
the line:

include/old/safe_macros.h:
#define SAFE_CLOSE(cleanup_fn, fildes)  \
        safe_close(__FILE__, __LINE__, (cleanup_fn), (fildes))

I'll send a patch for oldlib SAFE_CLOSE.

> 
> That doc also gives an example (in section 2.2.1) that
> says "Since global variables are initialized to zero we can
> just check that fd > 0 before we attempt to close it.",
> which is why I used 0 rather than -1. If current preferred
> test style has changed it would be nice to update the
> example code.

Hmm. Cyril, can we guarantee that testcase won't open fd 0?
Would that apply for both newlib and oldlib?

Regards,
Jan

> 
> thanks
> -- PMM
> 

[LTP] [PATCH] syscalls/mmap16: close open files in cleanup path

[Cyril Hrubis]

Hi!
> > after the SAFE_CLOSE() line? doc/test-writing-guidelines.txt
> > says "The SAFE_CLOSE() function also sets the passed file
> > descriptor to -1 after it's successfully closed.", so if
> > that's not correct we should fix the docs.
> 
> It's true for newlib, but not for oldlib. I think we should
> rather fix oldlib SAFE_CLOSE to match the docs, so I dropped
> the line:
> 
> include/old/safe_macros.h:
> #define SAFE_CLOSE(cleanup_fn, fildes)  \
>         safe_close(__FILE__, __LINE__, (cleanup_fn), (fildes))
> 
> I'll send a patch for oldlib SAFE_CLOSE.

I avoided changing the oldlib in order not to introduce any regressions.
There may be code that depends on the fd > 0 even after the close, it's
unlikely but possible.

I'm OK with changing it, I just avoided to do so in the patch that added
the new library API.

> > That doc also gives an example (in section 2.2.1) that
> > says "Since global variables are initialized to zero we can
> > just check that fd > 0 before we attempt to close it.",
> > which is why I used 0 rather than -1. If current preferred
> > test style has changed it would be nice to update the
> > example code.
> 
> Hmm. Cyril, can we guarantee that testcase won't open fd 0?
> Would that apply for both newlib and oldlib?

Well the fd 0 always points to STDIN, so unless the test actively closes
it this will work. There may be testcases that does that, and would
require special treatement, but I'm pretty sure that these are in single
numbers.

-- 
Cyril Hrubis
chrubis@suse.cz