[LTP] [PATCH 0/2] syscalls/fork: Rewrite fork{07, 08} to a proper synchronization

[LTP] [PATCH 0/2] syscalls/fork: Rewrite fork{07, 08} to a proper synchronization

[Xie Ziyao]

Fixes: #774

Xie Ziyao (2):
  fork07: Rewrite the test to a proper synchronization
  fork08: Rewrite the test to a proper synchronization

 testcases/kernel/syscalls/fork/fork07.c | 253 +++++++-----------------
 testcases/kernel/syscalls/fork/fork08.c | 198 +++++--------------
 2 files changed, 118 insertions(+), 333 deletions(-)

--
2.17.1

[LTP] [PATCH 1/2] fork07: Rewrite the test to a proper synchronization

[Xie Ziyao]

Rewrite fork07 to a proper synchronization with the new API.

Fixes: #774
Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
 testcases/kernel/syscalls/fork/fork07.c | 253 +++++++-----------------
 1 file changed, 68 insertions(+), 185 deletions(-)

diff --git a/testcases/kernel/syscalls/fork/fork07.c b/testcases/kernel/syscalls/fork/fork07.c
index e596867c3..751d64c5a 100644
--- a/testcases/kernel/syscalls/fork/fork07.c
+++ b/testcases/kernel/syscalls/fork/fork07.c
@@ -1,212 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
+ * Copyright (c) International Business Machines  Corp., 2001
+ * 07/2001 Ported by Wayne Boyer
+ * 07/2002 Limited forking and split "infinite forks" testcase to fork12.c by
+ * Nate Straz
+ * Copyright (c) 2021 Xie Ziyao <xieziyao@huawei.com>
+ */
+
+/*\
+ * [Description]
  *
- *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- *
- * NAME
- *	fork07.c
- *
- * DESCRIPTION
- *	Check that all children inherit parent's file descriptor
- *
- * ALGORITHM
- *	Parent opens a file, writes to it; forks Nforks children.
- *	Each child attempts to read the file then returns.
- *	Parent reports PASS if all children succeed.
- *
- * USAGE
- *	fork07
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *	07/2002 Limited forking and split "infinite forks" test case to
- *	        fork12.c by Nate Straz
- *
- * RESTRICTIONS
- *	None
+ * Check that all children inherit parent's file descriptor.
  */

-#include <stdio.h>
-#include <errno.h>
-#include <string.h>
+#include <stdlib.h>
 #include <sys/wait.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include "test.h"

-char *TCID = "fork07";
-int TST_TOTAL = 1;
+#include "tst_test.h"
+#include "tst_safe_stdio.h"

-static void help(void);
-static void setup(void);
-static void cleanup(void);
+#define NFORKS 100
+#define TESTFILE "testfile_fork07"

-static char pbuf[10];
-static char fnamebuf[40];
-
-static char *Nforkarg;
-static int Nflag;
-static int Nforks;
-static int vflag;
-
-static option_t options[] = {
-	{"N:", &Nflag, &Nforkarg},
-	{"v", &vflag, NULL},
-	{NULL, NULL, NULL}
-};
+static FILE *rea;
+static int status;

-int main(int ac, char **av)
+static void run(void)
 {
-	int status, forks, pid1;
+	int forks;
 	int ch_r_stat;
-	FILE *rea, *writ;
 	int c_pass, c_fail;

-	int lc;
-
-	rea = NULL;
-	writ = NULL;
-
-	tst_parse_opts(ac, av, options, &help);
-
-	if (Nflag) {
-		if (sscanf(Nforkarg, "%i", &Nforks) != 1)
-			tst_brkm(TBROK, cleanup,
-				 "--N option arg is not a number");
-	} else
-		Nforks = 100;
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		writ = fopen(fnamebuf, "w");
-		if (writ == NULL)
-			tst_resm(TFAIL | TERRNO, "fopen(.. \"w\") failed");
-		rea = fopen(fnamebuf, "r");
-		if (rea == NULL)
-			tst_resm(TFAIL | TERRNO, "fopen(.. \"r\") failed");
-
-		fprintf(writ, "abcdefghijklmnopqrstuv");
-		fflush(writ);
-		sleep(1);
-
-		if ((getc(rea)) != 'a')
-			tst_resm(TFAIL, "getc from read side was confused");
-
-		/* fork off the children */
-		tst_resm(TINFO, "Forking %d children", Nforks);
-		tst_old_flush();
-		for (forks = 0; forks < Nforks; forks++) {
-			pid1 = fork();
-			if (pid1 == 0) {
-				ch_r_stat = getc(rea);
-#ifdef DEBUG
-				tst_resm(TINFO, "Child got char: %c",
-					 ch_r_stat);
-				tst_resm(TINFO,
-					 "integer value of getc in child "
-					 "expected %d got %d", 'b', ch_r_stat);
-#endif
-				if (ch_r_stat == 'b') {
-					if (vflag) {
-						tst_resm(TINFO,
-							 "%6d: read correct character",
-							 getpid());
-					}
-					exit(0);
-				} else {
-					if (vflag) {
-						tst_resm(TINFO,
-							 "%6d: read '%c' instead of 'b'",
-							 getpid(),
-							 (char)ch_r_stat);
-					}
-					exit(1);
-				}
-			} else if (pid1 == -1)
-				tst_brkm(TBROK | TERRNO, cleanup,
-					 "fork failed");
-		}
-		tst_resm(TINFO, "Forked all %d children, now collecting",
-			 Nforks);
-
-		/* Collect all the kids and see how they did */
-
-		c_pass = c_fail = 0;
-		while (wait(&status) > 0) {
-			if (WIFEXITED(status) && WEXITSTATUS(status) == 0)
-				c_pass++;
-			else
-				c_fail++;
-			--forks;
+	rea = SAFE_FOPEN(TESTFILE, "r");
+	if ((getc(rea)) != 'a')
+		tst_res(TFAIL, "getc from read side was confused");
+
+	tst_flush();
+	for (forks = 0; forks < NFORKS; ++forks) {
+		if (!SAFE_FORK()) {
+			ch_r_stat = getc(rea);
+			if (ch_r_stat == 'b')
+				exit(0);
+
+			tst_res(TINFO, "%6d: read '%c' instead of 'b'",
+				getpid(), (char)ch_r_stat);
+			exit(1);
 		}
-		if (forks == 0) {
-			tst_resm(TINFO, "Collected all %d children", Nforks);
-			if (c_fail > 0)
-				tst_resm(TFAIL,
-					 "%d/%d children didn't read correctly from an inheritted fd",
-					 c_fail, Nforks);
-			else
-				tst_resm(TPASS,
-					 "%d/%d children read correctly from an inheritted fd",
-					 c_pass, Nforks);
-		} else if (forks > 0)
-			tst_brkm(TBROK, cleanup,
-				 "There should be %d more children to collect!",
-				 forks);
-		else
+	}

-			tst_brkm(TBROK, cleanup,
-				 "Collected %d more children then I should have!",
-				 abs(forks));
+	c_pass = c_fail = 0;
+	while (wait(&status) > 0) {
+		if (WIFEXITED(status) && WEXITSTATUS(status) == 0)
+			++c_pass;
+		else
+			++c_fail;
+		--forks;
 	}
-	fclose(writ);
-	fclose(rea);
-	cleanup();

-	tst_exit();
-}
+	if (forks != 0)
+		tst_brk(TBROK, "%d children are collected, expected %d",
+			NFORKS - forks, NFORKS);

-static void help(void)
-{
-	printf("  -N n    Create n children each iteration\n");
-	printf("  -v      Verbose mode\n");
+	if (c_fail > 0)
+		tst_res(TFAIL, "%d children read incorrectly", c_fail);
+	else
+		tst_res(TPASS, "all children read correctly");
+
+	SAFE_FCLOSE(rea);
 }

 static void setup(void)
 {
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	umask(0);
-	TEST_PAUSE;
-	tst_tmpdir();
-
-	strcpy(fnamebuf, "fork07.");
-	sprintf(pbuf, "%d", getpid());
-	strcat(fnamebuf, pbuf);
+	int fd;
+	char buf[27];
+
+	fd = SAFE_CREAT(TESTFILE, 00700);
+	sprintf(buf, "abcdefghijklmnopqrstuvwxyz");
+	SAFE_WRITE(1, fd, buf, strlen(buf));
+	SAFE_CLOSE(fd);
 }

 static void cleanup(void)
 {
-	int waitstatus;
-
-	/* collect our zombies */
-	while (wait(&waitstatus) > 0) ;
-
-	unlink(fnamebuf);
-	tst_rmdir();
+	tst_reap_children();
+	SAFE_UNLINK(TESTFILE);
 }
+
+static struct tst_test test = {
+	.forks_child = 1,
+	.needs_tmpdir = 1,
+	.cleanup = cleanup,
+	.setup = setup,
+	.test_all = run,
+};
--
2.17.1

[LTP] [PATCH 1/2] fork07: Rewrite the test to a proper synchronization

[Cyril Hrubis]

Hi!
The more I look at the test to more I realize how broken it is.

It starts with a wrong premise that file offset changes, as a
consequences of read(), are not propagated between the child and parent
process for a file descriptor that has been opened by parent and
propagated to a child on a fork(). This is not true at all as internally
in kernel the child and parent file descriptor will refer to the same
open file structure. And the test avoids failures by reading a byte from
a libc FILE in the parent, which of course caches data in the FILE
buffer so the first read() in libc reads the whole buffer. After that
each forked child will get the exact FILE structure in it's userspace
memory with the buffered data.

In conclusion this test has to be deleted and we should write a new one
from a scratch.

When we are asked to check if file descriptors are passed down correctly
between forks it makes much more sense to do it as follows:

* Parent writes N bytes (for example 'a') to a file in setup()
* Parent opens a file descriptor for reading pointing to that file
* Parent forks N children
  - each child reads a byte from the file
    checks that the byte is 'a' then exits
* Parent waits the all the children
* Parent checks that the end of file is reached
  for example by checking that read() from the
  file descriptor returns 0


And the other way around would be:

* Parent opens a file descriptor for writing pointing to an empty file
  (the file should be unlinked() before the open with O_CREAT)
* Parent forks N children
  - each child writes a sequence of bytes 'test' or something like this
* Parent waits the all the children
* Parent checks that the file is filled with N string 'test' that are
  not interleaved

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] fork08: Rewrite the test to a proper synchronization

[Xie Ziyao]

Rewrite fork08 to a proper synchronization with the new API.

Fixes: #774
Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
 testcases/kernel/syscalls/fork/fork08.c | 198 ++++++------------------
 1 file changed, 50 insertions(+), 148 deletions(-)

diff --git a/testcases/kernel/syscalls/fork/fork08.c b/testcases/kernel/syscalls/fork/fork08.c
index 1123f7473..082803f47 100644
--- a/testcases/kernel/syscalls/fork/fork08.c
+++ b/testcases/kernel/syscalls/fork/fork08.c
@@ -1,172 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- *
- * NAME
- *	fork08.c
- *
- * DESCRIPTION
- *	Check if the parent's file descriptors are affected by
- *	actions in the child; they should not be.
- *
- * ALGORITHM
- *	Parent opens a file.
- *	Forks a child which closes a file.
- *	Parent forks a second child which attempts to read the (closed)
- *	file.
- *
- * USAGE
- *	fork08
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
+ * Copyright (c) International Business Machines  Corp., 2001
+ * 07/2001 Ported by Wayne Boyer
+ * Copyright (c) 2021 Xie Ziyao <xieziyao@huawei.com>
+ */
+
+/*\
+ * [Description]
  *
- * RESTRICTIONS
- *	None
+ * Check that the parent's file descriptors will not be affected by actions
+ * in the child.
  */

-#include <sys/types.h>
-#include <sys/wait.h>
-#include <sys/stat.h>
-#include <stdio.h>
-#include "test.h"
+#include <stdlib.h>

-char *TCID = "fork08";
-int TST_TOTAL = 1;
+#include "tst_test.h"
+#include "tst_safe_stdio.h"

-static void setup(void);
-static void cleanup(void);
+#define TESTFILE "testfile_fork08"

-static char pbuf[10];
-static char fnamebuf[40];
+static FILE *rea;

-int main(int ac, char **av)
+static void run(void)
 {
-	int status, count, forks, pid1;
 	int ch_r_stat;
-	FILE *rea, *writ;
-
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);

-	setup();
+	rea = SAFE_FOPEN(TESTFILE, "r");
+	if ((getc(rea)) != 'a')
+		tst_res(TFAIL, "getc from read side was confused");
+	tst_flush();

-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		writ = fopen(fnamebuf, "w");
-		if (writ == NULL)
-			tst_resm(TFAIL, "failed to fopen file for write");
-		rea = fopen(fnamebuf, "r");
-		if (rea == NULL)
-			tst_resm(TFAIL, "failed to fopen file for read");
-
-		fprintf(writ, "abcdefghijklmnopqrstuv");
-		fflush(writ);
-		sleep(1);
-
-		if ((getc(rea)) != 'a')
-			tst_resm(TFAIL, "getc from read side was confused");
-
-		forks = 0;
-
-forkone:
-		++forks;
-
-		pid1 = fork();
-		if (pid1 != 0) {
-			tst_resm(TINFO, "parent forksval: %d", forks);
-
-			if ((pid1 != (-1)) && (forks < 2))
-				goto forkone;
-			else if (pid1 < 0)
-				tst_resm(TINFO, "Fork failed");
-		} else {	/* child */
-			/*
-			 * If first child close the file descriptor for the
-			 * read stream
-			 */
-			if (forks == 1) {
-				if ((fclose(rea)) == -1) {
-					tst_resm(TFAIL, "error in first child"
-						 " closing fildes");
-				}
-				_exit(0);
-			}
-
-			/*
-			 * If second child attempt to read from the file
-			 */
-			else if (forks == 2) {
-				ch_r_stat = getc(rea);
-				tst_resm(TINFO, "second child got char: %c",
-					 ch_r_stat);
-				if (ch_r_stat == 'b') {
-					tst_resm(TPASS, "Test passed in child "
-						 "number %d", forks);
-					exit(0);
-				} else if (ch_r_stat == EOF) {
-					tst_resm(TFAIL, "Second child got "
-						 "EOF");
-					exit(-1);
-				} else {
-					tst_resm(TFAIL, "test failed in child "
-						 "no %d", forks);
-					exit(-1);
-				}
-			} else {	/* end of second child */
-				tst_resm(TINFO, "forksnumber: %d", forks);
-				exit(3);
-			}
-		}
+	if (!SAFE_FORK()) {
+		SAFE_FCLOSE(rea);
+		exit(0);
+	}
+	tst_reap_children();

-		for (count = 0; count <= forks; count++) {
-			wait(&status);
-			tst_resm(TINFO, "exit status of wait "
-				 " expected 0 got %d", status);
-			status >>= 8;
-			if (status == 0)
-				tst_resm(TPASS, "parent test PASSED");
-			else
-				tst_resm(TFAIL, "parent test FAILED");
+	if (!SAFE_FORK()) {
+		ch_r_stat = getc(rea);
+		if (ch_r_stat == 'b') {
+			tst_res(TPASS, "%6d: read correctly", getpid());
+			exit(0);
 		}

-		tst_resm(TINFO, "Number of processes forked is %d", forks);
-		fclose(rea);
-		fclose(writ);
+		if (ch_r_stat == EOF)
+			tst_res(TFAIL, "%6d: got EOF", getpid());
+		else
+			tst_res(TFAIL, "%6d: read '%c' instead of 'b'",
+				getpid(), (char)ch_r_stat);
+		exit(-1);
 	}
+	tst_reap_children();

-	cleanup();
-	tst_exit();
+	SAFE_FCLOSE(rea);
 }

 static void setup(void)
 {
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	umask(0);
-	TEST_PAUSE;
-	tst_tmpdir();
+	int fd;
+	char buf[27];

-	strcpy(fnamebuf, "fork07.");
-	sprintf(pbuf, "%d", getpid());
-	strcat(fnamebuf, pbuf);
+	fd = SAFE_CREAT(TESTFILE, 00700);
+	sprintf(buf, "abcdefghijklmnopqrstuvwxyz");
+	SAFE_WRITE(1, fd, buf, strlen(buf));
+	SAFE_CLOSE(fd);
 }

-static void cleanup(void)
-{
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.forks_child = 1,
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test_all = run,
+};
--
2.17.1

[LTP] [PATCH 2/2] fork08: Rewrite the test to a proper synchronization

[Cyril Hrubis]

Hi!
Can we please get rid of the FILE and use file descriptors instead?

Here as well the test is invalid since the whole file is read into libc
buffers on the first call to getc().

-- 
Cyril Hrubis
chrubis@suse.cz