diff mbox series

[v3,2/5] syscalls/clone03: Convert to new API

Message ID 20210923085224.868-2-zhanglianjie@uniontech.com
State Changes Requested
Headers show
Series [v3,1/5] syscalls/clone02: Convert to new API | expand

Commit Message

zhanglianjie Sept. 23, 2021, 8:52 a.m. UTC
Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>

--
2.20.1

Comments

Richard Palethorpe Sept. 24, 2021, 9:19 a.m. UTC | #1
Hello,

This is a big improvement. However there are some things from the old
test which can be improved.

> +static void verify_clone(void)
> +{
> +	int child_pid;
>
> -		/* Close read end from parent */
> -		if ((close(pfd[0])) == -1)
> -			tst_resm(TWARN | TERRNO, "close(pfd[0]) failed");
> +	TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, child_fn, NULL, CHILD_STACK_SIZE,
> +				child_stack));

tst_clone is the new API.

>
> -		/* Get child's pid from pid string */
> -		child_pid = atoi(buff);
> +	if (!TST_PASS)
> +		return;
>
> -		if (TEST_RETURN == child_pid)
> -			tst_resm(TPASS, "Test passed");
> -		else
> -			tst_resm(TFAIL, "Test failed");
> +	tst_reap_children();
>
> -		if ((wait(&status)) == -1)
> -			tst_brkm(TBROK | TERRNO, cleanup,
> -				 "wait failed, status: %d", status);
> -	}
> +	child_pid = atoi(channel);

atoi is deprecated (see the man page).

>
> -	free(child_stack);
> -
> -	cleanup();
> -	tst_exit();
> +	TST_EXP_PASS(!(TST_RET == child_pid), "pid(%d)", child_pid);
>  }
>
>  static void setup(void)
>  {
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	TEST_PAUSE;
> +	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
> +	channel = SAFE_MMAP(NULL, 10, PROT_READ | PROT_WRITE,
> +				MAP_SHARED | MAP_ANONYMOUS, -1, 0);

You could mmap a region needed for pid_t and just read and write it like
a normal variable.
Cyril Hrubis Oct. 11, 2021, 3:02 p.m. UTC | #2
Hi!
> > +static void verify_clone(void)
> > +{
> > +	int child_pid;
> >
> > -		/* Close read end from parent */
> > -		if ((close(pfd[0])) == -1)
> > -			tst_resm(TWARN | TERRNO, "close(pfd[0]) failed");
> > +	TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, child_fn, NULL, CHILD_STACK_SIZE,
> > +				child_stack));
> 
> tst_clone is the new API.

Actually we can't use it as it is becuase:

- these tests are tests fof clone() not for clone3() so we really have
  to use the old syscall

- even if we added flag to tst_clone to force the older syscall we would
  have to add size parameter to the structure since we need to test the
  case where we pass the stack explicitly as well

I guess that it would be better to do this eventually but I do not want
to block the cleanup because of this. We can fix this later on.

> >
> > -		/* Get child's pid from pid string */
> > -		child_pid = atoi(buff);
> > +	if (!TST_PASS)
> > +		return;
> >
> > -		if (TEST_RETURN == child_pid)
> > -			tst_resm(TPASS, "Test passed");
> > -		else
> > -			tst_resm(TFAIL, "Test failed");
> > +	tst_reap_children();
> >
> > -		if ((wait(&status)) == -1)
> > -			tst_brkm(TBROK | TERRNO, cleanup,
> > -				 "wait failed, status: %d", status);
> > -	}
> > +	child_pid = atoi(channel);
> 
> atoi is deprecated (see the man page).
> 
> >
> > -	free(child_stack);
> > -
> > -	cleanup();
> > -	tst_exit();
> > +	TST_EXP_PASS(!(TST_RET == child_pid), "pid(%d)", child_pid);
> >  }
> >
> >  static void setup(void)
> >  {
> > -	tst_sig(FORK, DEF_HANDLER, cleanup);
> > -	TEST_PAUSE;
> > +	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
> > +	channel = SAFE_MMAP(NULL, 10, PROT_READ | PROT_WRITE,
> > +				MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> 
> You could mmap a region needed for pid_t and just read and write it like
> a normal variable.

That was my point when I suggested mmap()

it should really be:

static int *child_pid;


...
	child_pid = SAFE_MMAP(NULL, sizeof(*child_pid), ....);
...

Then we can just do *child_pid = foo and if (*child_pid == bar) in the
code.
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/clone/clone03.c b/testcases/kernel/syscalls/clone/clone03.c
index f4216ead8..81d6ee649 100644
--- a/testcases/kernel/syscalls/clone/clone03.c
+++ b/testcases/kernel/syscalls/clone/clone03.c
@@ -1,147 +1,61 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (c) Wipro Technologies Ltd, 2002.  All Rights Reserved.
  * Copyright (c) 2012 Wanlong Gao <gaowanlong@cn.fujitsu.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
  */
-/*
+
+/*\
+ * [Description]
  *	Check for equality of pid of child & return value of clone(2)
- *	Test:
- *	 Open a pipe.
- *	 Loop if the proper options are given.
- *	  Call clone(2) called without SIGCHLD
- *
- *	  CHILD:
- *		writes the pid to pipe
- *	  PARENT:
- *		reads child'd pid from pipe
- *		if child's pid == return value from clone(2)
- *			Test passed
- *		else
- *			test failed
  */
-
-#if defined UCLINUX && !__THROW
-/* workaround for libc bug */
-#define __THROW
-#endif
-
-#include <errno.h>
-#include <sched.h>
-#include <sys/wait.h>
-#include "test.h"
-
+#include <stdio.h>
+#include <stdlib.h>
+#include "tst_test.h"
 #include "clone_platform.h"

-static void setup(void);
-static void cleanup(void);
-static int child_fn();
-
-static int pfd[2];
+static void *child_stack;
+static char *channel;

-char *TCID = "clone03";
-int TST_TOTAL = 1;
-
-int main(int ac, char **av)
+static int child_fn(void *arg LTP_ATTRIBUTE_UNUSED)
 {
+	sprintf(channel, "%d", getpid());
+	exit(0);
+}

-	int lc;
-	void *child_stack;
-	char buff[10];
-	int child_pid, status;
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	/* Allocate stack for child */
-	child_stack = malloc(CHILD_STACK_SIZE);
-	if (child_stack == NULL)
-		tst_brkm(TBROK, cleanup, "Cannot allocate stack for child");
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		if ((pipe(pfd)) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup, "pipe failed");
-
-		TEST(ltp_clone(SIGCHLD, child_fn, NULL, CHILD_STACK_SIZE,
-			       child_stack));
-
-		if (TEST_RETURN == -1)
-			tst_brkm(TFAIL | TTERRNO, cleanup, "clone() failed");
-
-		/* close write end from parent */
-		if ((close(pfd[1])) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup,
-				 "close(pfd[1]) failed");
-
-		/* Read pid from read end */
-		if ((read(pfd[0], buff, sizeof(buff))) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup,
-				 "read from pipe failed");
+static void verify_clone(void)
+{
+	int child_pid;

-		/* Close read end from parent */
-		if ((close(pfd[0])) == -1)
-			tst_resm(TWARN | TERRNO, "close(pfd[0]) failed");
+	TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, child_fn, NULL, CHILD_STACK_SIZE,
+				child_stack));

-		/* Get child's pid from pid string */
-		child_pid = atoi(buff);
+	if (!TST_PASS)
+		return;

-		if (TEST_RETURN == child_pid)
-			tst_resm(TPASS, "Test passed");
-		else
-			tst_resm(TFAIL, "Test failed");
+	tst_reap_children();

-		if ((wait(&status)) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup,
-				 "wait failed, status: %d", status);
-	}
+	child_pid = atoi(channel);

-	free(child_stack);
-
-	cleanup();
-	tst_exit();
+	TST_EXP_PASS(!(TST_RET == child_pid), "pid(%d)", child_pid);
 }

 static void setup(void)
 {
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	TEST_PAUSE;
+	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
+	channel = SAFE_MMAP(NULL, 10, PROT_READ | PROT_WRITE,
+				MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 }

 static void cleanup(void)
 {
-}
-
-static int child_fn(void)
-{
-	char pid[10];
-
-	/* Close read end from child */
-	if ((close(pfd[0])) == -1)
-		perror("close(pfd[0]) failed");
-
-	sprintf(pid, "%d", getpid());
-
-	/* Write pid string to pipe */
-	if ((write(pfd[1], pid, sizeof(pid))) == -1)
-		perror("write to pipe failed");
-
-	/* Close write end of pipe from child */
-	if ((close(pfd[1])) == -1)
-		perror("close(pfd[1]) failed");
+	free(child_stack);

-	exit(1);
+	if (channel)
+		SAFE_MUNMAP(channel, 10);
 }
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = verify_clone,
+	.cleanup = cleanup,
+};