[LTP] [PATCH v1] Refactor pidns05 test using new LTP API

[LTP] [PATCH v1] Refactor pidns05 test using new LTP API

[Andrea Cervesato via ltp]

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/containers/pidns/pidns05.c | 288 ++++++--------------
 1 file changed, 78 insertions(+), 210 deletions(-)

diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
index 79e146e36..1c588991b 100644
--- a/testcases/kernel/containers/pidns/pidns05.c
+++ b/testcases/kernel/containers/pidns/pidns05.c
@@ -1,256 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
-* Copyright (c) International Business Machines Corp., 2007
-* 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
-*
-***************************************************************************
-*
-* Assertion:
-*   a) Create a  container.
-*   b) Create many levels of child containers inside this container.
-*   c) Now do kill -9 init , outside of the container.
-*   d) This should kill all the child containers.
-*      (containers created at the level below)
-*
-* Description:
-* 1. Parent process clone a process with flag CLONE_NEWPID
-* 2. The container will recursively loop and creates 4 more containers.
-* 3. All the container init's  goes into sleep(), waiting to be terminated.
-* 4. The parent process will kill child[3] by passing SIGKILL
-* 5. Now parent process, verifies the child containers 4 & 5 are destroyed.
-* 6. If they are killed then
-*	Test passed
-*  else Test failed.
-*
-* Test Name: pidns05
-*
-* History:
-*
-* FLAG DATE		NAME				DESCRIPTION
-* 31/10/08  Veerendra C <vechandr@in.ibm.com>	Verifies killing of NestedCont's
-*
-*******************************************************************************/
-#define _GNU_SOURCE 1
+ * Copyright (c) International Business Machines Corp., 2007
+ *		08/10/08 Veerendra C <vechandr@in.ibm.com>
+ * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Clone a process with CLONE_NEWPID flag and create many levels of child
+ * containers. Then kill container init process from parent and check if all
+ * containers have been killed.
+ */
+
 #include <sys/wait.h>
-#include <assert.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <string.h>
-#include <errno.h>
-#include "pidns_helper.h"
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
+#include "lapi/namespaces_constants.h"
 
-#define INIT_PID	1
-#define CINIT_PID	1
-#define PARENT_PID	0
 #define MAX_DEPTH	5
 
-char *TCID = "pidns05";
-int TST_TOTAL = 1;
-int fd[2];
+static pid_t pid_max;
 
-int max_pid(void)
+static int child_func(void *arg)
 {
-	FILE *fp;
 	int ret;
+	int *level;
+	pid_t cpid, ppid;
+
+	cpid = getpid();
+	ppid = getppid();
 
-	fp = fopen("/proc/sys/kernel/pid_max", "r");
-	if (fp != NULL) {
-		fscanf(fp, "%d", &ret);
-		fclose(fp);
-	} else {
-		tst_resm(TBROK, "Cannot open /proc/sys/kernel/pid_max");
-		ret = -1;
+	if (cpid != 1 || ppid != 0) {
+		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
+		return 1;
 	}
-	return ret;
+
+	level = (int *)arg;
+
+	if (*level >= MAX_DEPTH) {
+		TST_CHECKPOINT_WAKE(0);
+		return 0;
+	}
+
+	(*level)++;
+
+	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, level);
+	if (ret < 0)
+		tst_brk(TBROK | TERRNO, "clone failed");
+
+	pause();
+
+	return 0;
 }
 
-/* find_cinit_pids() iteratively finds the pid's having same PGID as its parent.
- * Input parameter - Accepts pointer to pid_t : To copy the pid's matching.
- * Returns - the number of pids matched.
-*/
-int find_cinit_pids(pid_t * pids)
+static int find_cinit_pids(pid_t *pids)
 {
-	int next = 0, pid_max, i;
+	int i;
+	int next = 0;
 	pid_t parentpid, pgid, pgid2;
 
-	pid_max = max_pid();
 	parentpid = getpid();
 	pgid = getpgid(parentpid);
 
-	/* The loop breaks, when the loop counter reaches the parentpid value */
-	for (i = parentpid + 1; i != parentpid; i++) {
-		if (i > pid_max)
-			i = 2;
-
+	for (i = parentpid + 1; i < pid_max; i++) {
 		pgid2 = getpgid(i);
+
 		if (pgid2 == pgid) {
 			pids[next] = i;
 			next++;
 		}
 	}
+
 	return next;
 }
 
-/*
-* create_nested_container() Recursively create MAX_DEPTH nested containers
-*/
-int create_nested_container(void *vtest)
+static void setup(void)
 {
-	int exit_val;
-	int ret, count, *level;
-	pid_t cpid, ppid;
-	cpid = getpid();
-	ppid = getppid();
-	char mesg[] = "Nested Containers are created";
-
-	level = (int *)vtest;
-	count = *level;
-
-	/* Child process closes up read side of pipe */
-	close(fd[0]);
-
-	/* Comparing the values to make sure pidns is created correctly */
-	if (cpid != CINIT_PID || ppid != PARENT_PID) {
-		printf("Got unexpected cpid and/or ppid (cpid=%d ppid=%d)\n",
-		       cpid, ppid);
-		exit_val = 1;
-	}
-	if (count > 1) {
-		count--;
-		ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
-					    create_nested_container,
-					    (void *)&count);
-		if (ret == -1) {
-			printf("clone failed; errno = %d : %s\n",
-			       ret, strerror(ret));
-			exit_val = 1;
-		} else
-			exit_val = 0;
-	} else {
-		/* Sending mesg, 'Nested containers created' through the pipe */
-		write(fd[1], mesg, (strlen(mesg) + 1));
-		exit_val = 0;
-	}
-
-	close(fd[1]);
-	pause();
-
-	return exit_val;
+	SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &pid_max);
 }
 
-void kill_nested_containers()
+static void run(void)
 {
-	int orig_count, new_count, status = 0, i;
+	int ret, i;
+	int status;
+	int children;
+	int level = 0;
 	pid_t pids[MAX_DEPTH];
 	pid_t pids_new[MAX_DEPTH];
 
-	orig_count = find_cinit_pids(pids);
-	kill(pids[MAX_DEPTH - 3], SIGKILL);
-	sleep(1);
-
-	/* After killing child container, getting the New PID list */
-	new_count = find_cinit_pids(pids_new);
+	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, &level);
+	if (ret < 0)
+		tst_brk(TBROK | TERRNO, "clone failed");
 
-	/* Verifying that the child containers were destroyed when parent is killed */
-	if (orig_count - 2 != new_count)
-		status = -1;
+	TST_CHECKPOINT_WAIT(0);
 
-	for (i = 0; i < new_count; i++) {
-		if (pids[i] != pids_new[i])
-			status = -1;
-	}
+	find_cinit_pids(pids);
 
-	if (status == 0)
-		tst_resm(TPASS, "The number of containers killed are %d",
-			 orig_count - new_count);
-	else
-		tst_resm(TFAIL, "Failed to kill the sub-containers of "
-			 "the container %d", pids[MAX_DEPTH - 3]);
-
-	/* Loops through the containers created to exit from sleep() */
-	for (i = 0; i < MAX_DEPTH; i++) {
-		kill(pids[i], SIGKILL);
-		waitpid(pids[i], &status, 0);
-	}
-}
+	SAFE_KILL(pids[0], SIGKILL);
 
-static void setup(void)
-{
-	tst_require_root();
-	check_newpid();
-}
+	TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);
 
-int main(void)
-{
-	int ret, nbytes, status;
-	char readbuffer[80];
-	pid_t pid, pgid;
-	int count = MAX_DEPTH;
+	children = find_cinit_pids(pids_new);
 
-	setup();
+	if (children > 0) {
+		tst_res(TFAIL, "%d children left after sending SIGKILL", children);
 
-	/*
-	 * XXX (garrcoop): why in the hell is this fork-wait written this way?
-	 * This doesn't add up with the pattern used for the rest of the tests,
-	 * so I'm pretty damn sure this test is written incorrectly.
-	 */
-	pid = fork();
-	if (pid == -1) {
-		tst_brkm(TBROK | TERRNO, NULL, "fork failed");
-	} else if (pid != 0) {
-		/*
-		 * NOTE: use waitpid so that we know we're waiting for the
-		 * _top-level_ child instead of a spawned subcontainer.
-		 *
-		 * XXX (garrcoop): Might want to mask SIGCHLD in the top-level
-		 * child too, or not *shrugs*.
-		 */
-		if (waitpid(pid, &status, 0) == -1) {
-			perror("wait failed");
+		for (i = 0; i < MAX_DEPTH; i++) {
+			kill(pids[i], SIGKILL);
+			waitpid(pids[i], &status, 0);
 		}
-		if (WIFEXITED(status))
-			exit(WEXITSTATUS(status));
-		else
-			exit(status);
-	}
 
-	/* To make all the containers share the same PGID as its parent */
-	setpgid(0, 0);
-
-	pid = getpid();
-	pgid = getpgid(pid);
-	SAFE_PIPE(NULL, fd);
-
-	TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
-				   create_nested_container, (void *)&count));
-	if (TEST_RETURN == -1) {
-		tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
+		return;
 	}
 
-	close(fd[1]);
-	/* Waiting for the MAX_DEPTH number of containers to be created */
-	nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
-	close(fd[0]);
-	if (nbytes > 0)
-		tst_resm(TINFO, " %d %s", MAX_DEPTH, readbuffer);
-	else
-		tst_brkm(TFAIL, NULL, "unable to create %d containers",
-			 MAX_DEPTH);
-
-	/* Kill the container created */
-	kill_nested_containers();
-
-	tst_exit();
+	tst_res(TPASS, "No children left after sending SIGKILL to the first child");
 }
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.needs_root = 1,
+	.needs_checkpoints = 1,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor pidns05 test using new LTP API

[Richard Palethorpe]

Hello,

Andrea Cervesato via ltp <ltp@lists.linux.it> writes:

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  testcases/kernel/containers/pidns/pidns05.c | 288 ++++++--------------
>  1 file changed, 78 insertions(+), 210 deletions(-)
>
> diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
> index 79e146e36..1c588991b 100644
> --- a/testcases/kernel/containers/pidns/pidns05.c
> +++ b/testcases/kernel/containers/pidns/pidns05.c
> @@ -1,256 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
> -* Copyright (c) International Business Machines Corp., 2007
> -* 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
> -*
> -***************************************************************************
> -*
> -* Assertion:
> -*   a) Create a  container.
> -*   b) Create many levels of child containers inside this container.
> -*   c) Now do kill -9 init , outside of the container.
> -*   d) This should kill all the child containers.
> -*      (containers created at the level below)
> -*
> -* Description:
> -* 1. Parent process clone a process with flag CLONE_NEWPID
> -* 2. The container will recursively loop and creates 4 more containers.
> -* 3. All the container init's  goes into sleep(), waiting to be terminated.
> -* 4. The parent process will kill child[3] by passing SIGKILL
> -* 5. Now parent process, verifies the child containers 4 & 5 are destroyed.
> -* 6. If they are killed then
> -*	Test passed
> -*  else Test failed.
> -*
> -* Test Name: pidns05
> -*
> -* History:
> -*
> -* FLAG DATE		NAME				DESCRIPTION
> -* 31/10/08  Veerendra C <vechandr@in.ibm.com>	Verifies killing of NestedCont's
> -*
> -*******************************************************************************/
> -#define _GNU_SOURCE 1
> + * Copyright (c) International Business Machines Corp., 2007
> + *		08/10/08 Veerendra C <vechandr@in.ibm.com>
> + * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Clone a process with CLONE_NEWPID flag and create many levels of child
> + * containers. Then kill container init process from parent and check if all
> + * containers have been killed.
> + */
> +
>  #include <sys/wait.h>
> -#include <assert.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <unistd.h>
> -#include <string.h>
> -#include <errno.h>
> -#include "pidns_helper.h"
> -#include "test.h"
> -#include "safe_macros.h"
> +#include "tst_test.h"
> +#include "lapi/namespaces_constants.h"
>  
> -#define INIT_PID	1
> -#define CINIT_PID	1
> -#define PARENT_PID	0
>  #define MAX_DEPTH	5
>  
> -char *TCID = "pidns05";
> -int TST_TOTAL = 1;
> -int fd[2];
> +static pid_t pid_max;
>  
> -int max_pid(void)
> +static int child_func(void *arg)
>  {
> -	FILE *fp;
>  	int ret;
> +	int *level;
> +	pid_t cpid, ppid;
> +
> +	cpid = getpid();
> +	ppid = getppid();
>  
> -	fp = fopen("/proc/sys/kernel/pid_max", "r");
> -	if (fp != NULL) {
> -		fscanf(fp, "%d", &ret);
> -		fclose(fp);
> -	} else {
> -		tst_resm(TBROK, "Cannot open /proc/sys/kernel/pid_max");
> -		ret = -1;
> +	if (cpid != 1 || ppid != 0) {
> +		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
> +		return 1;
>  	}
> -	return ret;
> +
> +	level = (int *)arg;
> +
> +	if (*level >= MAX_DEPTH) {
> +		TST_CHECKPOINT_WAKE(0);
> +		return 0;
> +	}
> +
> +	(*level)++;
> +
> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func,
> level);

Again, ltp_clone should be converted to tst_clone.

> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO, "clone failed");
> +
> +	pause();
> +
> +	return 0;
>  }
>  
> -/* find_cinit_pids() iteratively finds the pid's having same PGID as its parent.
> - * Input parameter - Accepts pointer to pid_t : To copy the pid's matching.
> - * Returns - the number of pids matched.
> -*/
> -int find_cinit_pids(pid_t * pids)
> +static int find_cinit_pids(pid_t *pids)
>  {
> -	int next = 0, pid_max, i;
> +	int i;
> +	int next = 0;
>  	pid_t parentpid, pgid, pgid2;
>  
> -	pid_max = max_pid();
>  	parentpid = getpid();
>  	pgid = getpgid(parentpid);
>  
> -	/* The loop breaks, when the loop counter reaches the parentpid value */
> -	for (i = parentpid + 1; i != parentpid; i++) {
> -		if (i > pid_max)
> -			i = 2;
> -
> +	for (i = parentpid + 1; i < pid_max; i++) {
>  		pgid2 = getpgid(i);
> +
>  		if (pgid2 == pgid) {
>  			pids[next] = i;
>  			next++;
>  		}
>  	}
> +
>  	return next;
>  }
>  
> -/*
> -* create_nested_container() Recursively create MAX_DEPTH nested containers
> -*/
> -int create_nested_container(void *vtest)
> +static void setup(void)
>  {
> -	int exit_val;
> -	int ret, count, *level;
> -	pid_t cpid, ppid;
> -	cpid = getpid();
> -	ppid = getppid();
> -	char mesg[] = "Nested Containers are created";
> -
> -	level = (int *)vtest;
> -	count = *level;
> -
> -	/* Child process closes up read side of pipe */
> -	close(fd[0]);
> -
> -	/* Comparing the values to make sure pidns is created correctly */
> -	if (cpid != CINIT_PID || ppid != PARENT_PID) {
> -		printf("Got unexpected cpid and/or ppid (cpid=%d ppid=%d)\n",
> -		       cpid, ppid);
> -		exit_val = 1;
> -	}
> -	if (count > 1) {
> -		count--;
> -		ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
> -					    create_nested_container,
> -					    (void *)&count);
> -		if (ret == -1) {
> -			printf("clone failed; errno = %d : %s\n",
> -			       ret, strerror(ret));
> -			exit_val = 1;
> -		} else
> -			exit_val = 0;
> -	} else {
> -		/* Sending mesg, 'Nested containers created' through the pipe */
> -		write(fd[1], mesg, (strlen(mesg) + 1));
> -		exit_val = 0;
> -	}
> -
> -	close(fd[1]);
> -	pause();
> -
> -	return exit_val;
> +	SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &pid_max);
>  }
>  
> -void kill_nested_containers()
> +static void run(void)
>  {
> -	int orig_count, new_count, status = 0, i;
> +	int ret, i;
> +	int status;
> +	int children;
> +	int level = 0;
>  	pid_t pids[MAX_DEPTH];
>  	pid_t pids_new[MAX_DEPTH];
>  
> -	orig_count = find_cinit_pids(pids);
> -	kill(pids[MAX_DEPTH - 3], SIGKILL);
> -	sleep(1);
> -
> -	/* After killing child container, getting the New PID list */
> -	new_count = find_cinit_pids(pids_new);
> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, &level);
> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO, "clone failed");
>  
> -	/* Verifying that the child containers were destroyed when parent is killed */
> -	if (orig_count - 2 != new_count)
> -		status = -1;
> +	TST_CHECKPOINT_WAIT(0);
>  
> -	for (i = 0; i < new_count; i++) {
> -		if (pids[i] != pids_new[i])
> -			status = -1;
> -	}
> +	find_cinit_pids(pids);
>  
> -	if (status == 0)
> -		tst_resm(TPASS, "The number of containers killed are %d",
> -			 orig_count - new_count);
> -	else
> -		tst_resm(TFAIL, "Failed to kill the sub-containers of "
> -			 "the container %d", pids[MAX_DEPTH - 3]);
> -
> -	/* Loops through the containers created to exit from sleep() */
> -	for (i = 0; i < MAX_DEPTH; i++) {
> -		kill(pids[i], SIGKILL);
> -		waitpid(pids[i], &status, 0);
> -	}
> -}
> +	SAFE_KILL(pids[0], SIGKILL);
>  
> -static void setup(void)
> -{
> -	tst_require_root();
> -	check_newpid();
> -}
> +	TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);
>  
> -int main(void)
> -{
> -	int ret, nbytes, status;
> -	char readbuffer[80];
> -	pid_t pid, pgid;
> -	int count = MAX_DEPTH;
> +	children = find_cinit_pids(pids_new);
>  
> -	setup();
> +	if (children > 0) {
> +		tst_res(TFAIL, "%d children left after sending SIGKILL", children);
>  
> -	/*
> -	 * XXX (garrcoop): why in the hell is this fork-wait written this way?
> -	 * This doesn't add up with the pattern used for the rest of the tests,
> -	 * so I'm pretty damn sure this test is written incorrectly.
> -	 */
> -	pid = fork();
> -	if (pid == -1) {
> -		tst_brkm(TBROK | TERRNO, NULL, "fork failed");
> -	} else if (pid != 0) {
> -		/*
> -		 * NOTE: use waitpid so that we know we're waiting for the
> -		 * _top-level_ child instead of a spawned subcontainer.
> -		 *
> -		 * XXX (garrcoop): Might want to mask SIGCHLD in the top-level
> -		 * child too, or not *shrugs*.
> -		 */
> -		if (waitpid(pid, &status, 0) == -1) {
> -			perror("wait failed");
> +		for (i = 0; i < MAX_DEPTH; i++) {
> +			kill(pids[i], SIGKILL);
> +			waitpid(pids[i], &status, 0);
>  		}
> -		if (WIFEXITED(status))
> -			exit(WEXITSTATUS(status));
> -		else
> -			exit(status);
> -	}
>  
> -	/* To make all the containers share the same PGID as its parent */
> -	setpgid(0, 0);
> -
> -	pid = getpid();
> -	pgid = getpgid(pid);
> -	SAFE_PIPE(NULL, fd);
> -
> -	TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
> -				   create_nested_container, (void *)&count));
> -	if (TEST_RETURN == -1) {
> -		tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
> +		return;
>  	}
>  
> -	close(fd[1]);
> -	/* Waiting for the MAX_DEPTH number of containers to be created */
> -	nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
> -	close(fd[0]);
> -	if (nbytes > 0)
> -		tst_resm(TINFO, " %d %s", MAX_DEPTH, readbuffer);
> -	else
> -		tst_brkm(TFAIL, NULL, "unable to create %d containers",
> -			 MAX_DEPTH);
> -
> -	/* Kill the container created */
> -	kill_nested_containers();
> -
> -	tst_exit();
> +	tst_res(TPASS, "No children left after sending SIGKILL to the first child");
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_root = 1,
> +	.needs_checkpoints = 1,
> +};
> -- 
> 2.35.3


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor pidns05 test using new LTP API

[Andrea Cervesato via ltp]

Hi!

Can we merge this patch anyway? ltp_clone should be added after with a 
single patch.

Andrea

On 10/11/22 11:56, Richard Palethorpe wrote:
> Hello,
>
> Andrea Cervesato via ltp <ltp@lists.linux.it> writes:
>
>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>> ---
>>   testcases/kernel/containers/pidns/pidns05.c | 288 ++++++--------------
>>   1 file changed, 78 insertions(+), 210 deletions(-)
>>
>> diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
>> index 79e146e36..1c588991b 100644
>> --- a/testcases/kernel/containers/pidns/pidns05.c
>> +++ b/testcases/kernel/containers/pidns/pidns05.c
>> @@ -1,256 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0
>>   /*
>> -* Copyright (c) International Business Machines Corp., 2007
>> -* 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
>> -*
>> -***************************************************************************
>> -*
>> -* Assertion:
>> -*   a) Create a  container.
>> -*   b) Create many levels of child containers inside this container.
>> -*   c) Now do kill -9 init , outside of the container.
>> -*   d) This should kill all the child containers.
>> -*      (containers created at the level below)
>> -*
>> -* Description:
>> -* 1. Parent process clone a process with flag CLONE_NEWPID
>> -* 2. The container will recursively loop and creates 4 more containers.
>> -* 3. All the container init's  goes into sleep(), waiting to be terminated.
>> -* 4. The parent process will kill child[3] by passing SIGKILL
>> -* 5. Now parent process, verifies the child containers 4 & 5 are destroyed.
>> -* 6. If they are killed then
>> -*	Test passed
>> -*  else Test failed.
>> -*
>> -* Test Name: pidns05
>> -*
>> -* History:
>> -*
>> -* FLAG DATE		NAME				DESCRIPTION
>> -* 31/10/08  Veerendra C <vechandr@in.ibm.com>	Verifies killing of NestedCont's
>> -*
>> -*******************************************************************************/
>> -#define _GNU_SOURCE 1
>> + * Copyright (c) International Business Machines Corp., 2007
>> + *		08/10/08 Veerendra C <vechandr@in.ibm.com>
>> + * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * Clone a process with CLONE_NEWPID flag and create many levels of child
>> + * containers. Then kill container init process from parent and check if all
>> + * containers have been killed.
>> + */
>> +
>>   #include <sys/wait.h>
>> -#include <assert.h>
>> -#include <stdio.h>
>> -#include <stdlib.h>
>> -#include <unistd.h>
>> -#include <string.h>
>> -#include <errno.h>
>> -#include "pidns_helper.h"
>> -#include "test.h"
>> -#include "safe_macros.h"
>> +#include "tst_test.h"
>> +#include "lapi/namespaces_constants.h"
>>   
>> -#define INIT_PID	1
>> -#define CINIT_PID	1
>> -#define PARENT_PID	0
>>   #define MAX_DEPTH	5
>>   
>> -char *TCID = "pidns05";
>> -int TST_TOTAL = 1;
>> -int fd[2];
>> +static pid_t pid_max;
>>   
>> -int max_pid(void)
>> +static int child_func(void *arg)
>>   {
>> -	FILE *fp;
>>   	int ret;
>> +	int *level;
>> +	pid_t cpid, ppid;
>> +
>> +	cpid = getpid();
>> +	ppid = getppid();
>>   
>> -	fp = fopen("/proc/sys/kernel/pid_max", "r");
>> -	if (fp != NULL) {
>> -		fscanf(fp, "%d", &ret);
>> -		fclose(fp);
>> -	} else {
>> -		tst_resm(TBROK, "Cannot open /proc/sys/kernel/pid_max");
>> -		ret = -1;
>> +	if (cpid != 1 || ppid != 0) {
>> +		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
>> +		return 1;
>>   	}
>> -	return ret;
>> +
>> +	level = (int *)arg;
>> +
>> +	if (*level >= MAX_DEPTH) {
>> +		TST_CHECKPOINT_WAKE(0);
>> +		return 0;
>> +	}
>> +
>> +	(*level)++;
>> +
>> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func,
>> level);
> Again, ltp_clone should be converted to tst_clone.
>
>> +	if (ret < 0)
>> +		tst_brk(TBROK | TERRNO, "clone failed");
>> +
>> +	pause();
>> +
>> +	return 0;
>>   }
>>   
>> -/* find_cinit_pids() iteratively finds the pid's having same PGID as its parent.
>> - * Input parameter - Accepts pointer to pid_t : To copy the pid's matching.
>> - * Returns - the number of pids matched.
>> -*/
>> -int find_cinit_pids(pid_t * pids)
>> +static int find_cinit_pids(pid_t *pids)
>>   {
>> -	int next = 0, pid_max, i;
>> +	int i;
>> +	int next = 0;
>>   	pid_t parentpid, pgid, pgid2;
>>   
>> -	pid_max = max_pid();
>>   	parentpid = getpid();
>>   	pgid = getpgid(parentpid);
>>   
>> -	/* The loop breaks, when the loop counter reaches the parentpid value */
>> -	for (i = parentpid + 1; i != parentpid; i++) {
>> -		if (i > pid_max)
>> -			i = 2;
>> -
>> +	for (i = parentpid + 1; i < pid_max; i++) {
>>   		pgid2 = getpgid(i);
>> +
>>   		if (pgid2 == pgid) {
>>   			pids[next] = i;
>>   			next++;
>>   		}
>>   	}
>> +
>>   	return next;
>>   }
>>   
>> -/*
>> -* create_nested_container() Recursively create MAX_DEPTH nested containers
>> -*/
>> -int create_nested_container(void *vtest)
>> +static void setup(void)
>>   {
>> -	int exit_val;
>> -	int ret, count, *level;
>> -	pid_t cpid, ppid;
>> -	cpid = getpid();
>> -	ppid = getppid();
>> -	char mesg[] = "Nested Containers are created";
>> -
>> -	level = (int *)vtest;
>> -	count = *level;
>> -
>> -	/* Child process closes up read side of pipe */
>> -	close(fd[0]);
>> -
>> -	/* Comparing the values to make sure pidns is created correctly */
>> -	if (cpid != CINIT_PID || ppid != PARENT_PID) {
>> -		printf("Got unexpected cpid and/or ppid (cpid=%d ppid=%d)\n",
>> -		       cpid, ppid);
>> -		exit_val = 1;
>> -	}
>> -	if (count > 1) {
>> -		count--;
>> -		ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>> -					    create_nested_container,
>> -					    (void *)&count);
>> -		if (ret == -1) {
>> -			printf("clone failed; errno = %d : %s\n",
>> -			       ret, strerror(ret));
>> -			exit_val = 1;
>> -		} else
>> -			exit_val = 0;
>> -	} else {
>> -		/* Sending mesg, 'Nested containers created' through the pipe */
>> -		write(fd[1], mesg, (strlen(mesg) + 1));
>> -		exit_val = 0;
>> -	}
>> -
>> -	close(fd[1]);
>> -	pause();
>> -
>> -	return exit_val;
>> +	SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &pid_max);
>>   }
>>   
>> -void kill_nested_containers()
>> +static void run(void)
>>   {
>> -	int orig_count, new_count, status = 0, i;
>> +	int ret, i;
>> +	int status;
>> +	int children;
>> +	int level = 0;
>>   	pid_t pids[MAX_DEPTH];
>>   	pid_t pids_new[MAX_DEPTH];
>>   
>> -	orig_count = find_cinit_pids(pids);
>> -	kill(pids[MAX_DEPTH - 3], SIGKILL);
>> -	sleep(1);
>> -
>> -	/* After killing child container, getting the New PID list */
>> -	new_count = find_cinit_pids(pids_new);
>> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, &level);
>> +	if (ret < 0)
>> +		tst_brk(TBROK | TERRNO, "clone failed");
>>   
>> -	/* Verifying that the child containers were destroyed when parent is killed */
>> -	if (orig_count - 2 != new_count)
>> -		status = -1;
>> +	TST_CHECKPOINT_WAIT(0);
>>   
>> -	for (i = 0; i < new_count; i++) {
>> -		if (pids[i] != pids_new[i])
>> -			status = -1;
>> -	}
>> +	find_cinit_pids(pids);
>>   
>> -	if (status == 0)
>> -		tst_resm(TPASS, "The number of containers killed are %d",
>> -			 orig_count - new_count);
>> -	else
>> -		tst_resm(TFAIL, "Failed to kill the sub-containers of "
>> -			 "the container %d", pids[MAX_DEPTH - 3]);
>> -
>> -	/* Loops through the containers created to exit from sleep() */
>> -	for (i = 0; i < MAX_DEPTH; i++) {
>> -		kill(pids[i], SIGKILL);
>> -		waitpid(pids[i], &status, 0);
>> -	}
>> -}
>> +	SAFE_KILL(pids[0], SIGKILL);
>>   
>> -static void setup(void)
>> -{
>> -	tst_require_root();
>> -	check_newpid();
>> -}
>> +	TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);
>>   
>> -int main(void)
>> -{
>> -	int ret, nbytes, status;
>> -	char readbuffer[80];
>> -	pid_t pid, pgid;
>> -	int count = MAX_DEPTH;
>> +	children = find_cinit_pids(pids_new);
>>   
>> -	setup();
>> +	if (children > 0) {
>> +		tst_res(TFAIL, "%d children left after sending SIGKILL", children);
>>   
>> -	/*
>> -	 * XXX (garrcoop): why in the hell is this fork-wait written this way?
>> -	 * This doesn't add up with the pattern used for the rest of the tests,
>> -	 * so I'm pretty damn sure this test is written incorrectly.
>> -	 */
>> -	pid = fork();
>> -	if (pid == -1) {
>> -		tst_brkm(TBROK | TERRNO, NULL, "fork failed");
>> -	} else if (pid != 0) {
>> -		/*
>> -		 * NOTE: use waitpid so that we know we're waiting for the
>> -		 * _top-level_ child instead of a spawned subcontainer.
>> -		 *
>> -		 * XXX (garrcoop): Might want to mask SIGCHLD in the top-level
>> -		 * child too, or not *shrugs*.
>> -		 */
>> -		if (waitpid(pid, &status, 0) == -1) {
>> -			perror("wait failed");
>> +		for (i = 0; i < MAX_DEPTH; i++) {
>> +			kill(pids[i], SIGKILL);
>> +			waitpid(pids[i], &status, 0);
>>   		}
>> -		if (WIFEXITED(status))
>> -			exit(WEXITSTATUS(status));
>> -		else
>> -			exit(status);
>> -	}
>>   
>> -	/* To make all the containers share the same PGID as its parent */
>> -	setpgid(0, 0);
>> -
>> -	pid = getpid();
>> -	pgid = getpgid(pid);
>> -	SAFE_PIPE(NULL, fd);
>> -
>> -	TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>> -				   create_nested_container, (void *)&count));
>> -	if (TEST_RETURN == -1) {
>> -		tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
>> +		return;
>>   	}
>>   
>> -	close(fd[1]);
>> -	/* Waiting for the MAX_DEPTH number of containers to be created */
>> -	nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
>> -	close(fd[0]);
>> -	if (nbytes > 0)
>> -		tst_resm(TINFO, " %d %s", MAX_DEPTH, readbuffer);
>> -	else
>> -		tst_brkm(TFAIL, NULL, "unable to create %d containers",
>> -			 MAX_DEPTH);
>> -
>> -	/* Kill the container created */
>> -	kill_nested_containers();
>> -
>> -	tst_exit();
>> +	tst_res(TPASS, "No children left after sending SIGKILL to the first child");
>>   }
>> +
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.setup = setup,
>> +	.needs_root = 1,
>> +	.needs_checkpoints = 1,
>> +};
>> -- 
>> 2.35.3
>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor pidns05 test using new LTP API

[Richard Palethorpe]

Hello,

Andrea Cervesato <andrea.cervesato@suse.com> writes:

> Hi!
>
> Can we merge this patch anyway? ltp_clone should be added after with a
> single patch.

I started work on an update to tst_clone so that it can replace most
intances of ltp_clone. However it appears that it can already replace
most instances of ltp_clone_quick.

AFAICT all you need to do is get rid of child_func and use tst_clone
like fork.

I don't remember if there is anything else in this patch that needs
reviewing, so there is no guarantee that when I look at this again it
will get merged.

So I don't see any advantage to deferring the change to tst_clone.

>
> Andrea
>
> On 10/11/22 11:56, Richard Palethorpe wrote:
>> Hello,
>>
>> Andrea Cervesato via ltp <ltp@lists.linux.it> writes:
>>
>>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>>> ---
>>>   testcases/kernel/containers/pidns/pidns05.c | 288 ++++++--------------
>>>   1 file changed, 78 insertions(+), 210 deletions(-)
>>>
>>> diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
>>> index 79e146e36..1c588991b 100644
>>> --- a/testcases/kernel/containers/pidns/pidns05.c
>>> +++ b/testcases/kernel/containers/pidns/pidns05.c
>>> @@ -1,256 +1,124 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>>   /*
>>> -* Copyright (c) International Business Machines Corp., 2007
>>> -* 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
>>> -*
>>> -***************************************************************************
>>> -*
>>> -* Assertion:
>>> -*   a) Create a  container.
>>> -*   b) Create many levels of child containers inside this container.
>>> -*   c) Now do kill -9 init , outside of the container.
>>> -*   d) This should kill all the child containers.
>>> -*      (containers created at the level below)
>>> -*
>>> -* Description:
>>> -* 1. Parent process clone a process with flag CLONE_NEWPID
>>> -* 2. The container will recursively loop and creates 4 more containers.
>>> -* 3. All the container init's  goes into sleep(), waiting to be terminated.
>>> -* 4. The parent process will kill child[3] by passing SIGKILL
>>> -* 5. Now parent process, verifies the child containers 4 & 5 are destroyed.
>>> -* 6. If they are killed then
>>> -*	Test passed
>>> -*  else Test failed.
>>> -*
>>> -* Test Name: pidns05
>>> -*
>>> -* History:
>>> -*
>>> -* FLAG DATE		NAME				DESCRIPTION
>>> -* 31/10/08  Veerendra C <vechandr@in.ibm.com>	Verifies killing of NestedCont's
>>> -*
>>> -*******************************************************************************/
>>> -#define _GNU_SOURCE 1
>>> + * Copyright (c) International Business Machines Corp., 2007
>>> + *		08/10/08 Veerendra C <vechandr@in.ibm.com>
>>> + * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
>>> + */
>>> +
>>> +/*\
>>> + * [Description]
>>> + *
>>> + * Clone a process with CLONE_NEWPID flag and create many levels of child
>>> + * containers. Then kill container init process from parent and check if all
>>> + * containers have been killed.
>>> + */
>>> +
>>>   #include <sys/wait.h>
>>> -#include <assert.h>
>>> -#include <stdio.h>
>>> -#include <stdlib.h>
>>> -#include <unistd.h>
>>> -#include <string.h>
>>> -#include <errno.h>
>>> -#include "pidns_helper.h"
>>> -#include "test.h"
>>> -#include "safe_macros.h"
>>> +#include "tst_test.h"
>>> +#include "lapi/namespaces_constants.h"
>>>   -#define INIT_PID	1
>>> -#define CINIT_PID	1
>>> -#define PARENT_PID	0
>>>   #define MAX_DEPTH	5
>>>   -char *TCID = "pidns05";
>>> -int TST_TOTAL = 1;
>>> -int fd[2];
>>> +static pid_t pid_max;
>>>   -int max_pid(void)
>>> +static int child_func(void *arg)
>>>   {
>>> -	FILE *fp;
>>>   	int ret;
>>> +	int *level;
>>> +	pid_t cpid, ppid;
>>> +
>>> +	cpid = getpid();
>>> +	ppid = getppid();
>>>   -	fp = fopen("/proc/sys/kernel/pid_max", "r");
>>> -	if (fp != NULL) {
>>> -		fscanf(fp, "%d", &ret);
>>> -		fclose(fp);
>>> -	} else {
>>> -		tst_resm(TBROK, "Cannot open /proc/sys/kernel/pid_max");
>>> -		ret = -1;
>>> +	if (cpid != 1 || ppid != 0) {
>>> +		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
>>> +		return 1;
>>>   	}
>>> -	return ret;
>>> +
>>> +	level = (int *)arg;
>>> +
>>> +	if (*level >= MAX_DEPTH) {
>>> +		TST_CHECKPOINT_WAKE(0);
>>> +		return 0;
>>> +	}
>>> +
>>> +	(*level)++;
>>> +
>>> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func,
>>> level);
>> Again, ltp_clone should be converted to tst_clone.
>>
>>> +	if (ret < 0)
>>> +		tst_brk(TBROK | TERRNO, "clone failed");
>>> +
>>> +	pause();
>>> +
>>> +	return 0;
>>>   }
>>>   -/* find_cinit_pids() iteratively finds the pid's having same
>>> PGID as its parent.
>>> - * Input parameter - Accepts pointer to pid_t : To copy the pid's matching.
>>> - * Returns - the number of pids matched.
>>> -*/
>>> -int find_cinit_pids(pid_t * pids)
>>> +static int find_cinit_pids(pid_t *pids)
>>>   {
>>> -	int next = 0, pid_max, i;
>>> +	int i;
>>> +	int next = 0;
>>>   	pid_t parentpid, pgid, pgid2;
>>>   -	pid_max = max_pid();
>>>   	parentpid = getpid();
>>>   	pgid = getpgid(parentpid);
>>>   -	/* The loop breaks, when the loop counter reaches the
>>> parentpid value */
>>> -	for (i = parentpid + 1; i != parentpid; i++) {
>>> -		if (i > pid_max)
>>> -			i = 2;
>>> -
>>> +	for (i = parentpid + 1; i < pid_max; i++) {
>>>   		pgid2 = getpgid(i);
>>> +
>>>   		if (pgid2 == pgid) {
>>>   			pids[next] = i;
>>>   			next++;
>>>   		}
>>>   	}
>>> +
>>>   	return next;
>>>   }
>>>   -/*
>>> -* create_nested_container() Recursively create MAX_DEPTH nested containers
>>> -*/
>>> -int create_nested_container(void *vtest)
>>> +static void setup(void)
>>>   {
>>> -	int exit_val;
>>> -	int ret, count, *level;
>>> -	pid_t cpid, ppid;
>>> -	cpid = getpid();
>>> -	ppid = getppid();
>>> -	char mesg[] = "Nested Containers are created";
>>> -
>>> -	level = (int *)vtest;
>>> -	count = *level;
>>> -
>>> -	/* Child process closes up read side of pipe */
>>> -	close(fd[0]);
>>> -
>>> -	/* Comparing the values to make sure pidns is created correctly */
>>> -	if (cpid != CINIT_PID || ppid != PARENT_PID) {
>>> -		printf("Got unexpected cpid and/or ppid (cpid=%d ppid=%d)\n",
>>> -		       cpid, ppid);
>>> -		exit_val = 1;
>>> -	}
>>> -	if (count > 1) {
>>> -		count--;
>>> -		ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>>> -					    create_nested_container,
>>> -					    (void *)&count);
>>> -		if (ret == -1) {
>>> -			printf("clone failed; errno = %d : %s\n",
>>> -			       ret, strerror(ret));
>>> -			exit_val = 1;
>>> -		} else
>>> -			exit_val = 0;
>>> -	} else {
>>> -		/* Sending mesg, 'Nested containers created' through the pipe */
>>> -		write(fd[1], mesg, (strlen(mesg) + 1));
>>> -		exit_val = 0;
>>> -	}
>>> -
>>> -	close(fd[1]);
>>> -	pause();
>>> -
>>> -	return exit_val;
>>> +	SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &pid_max);
>>>   }
>>>   -void kill_nested_containers()
>>> +static void run(void)
>>>   {
>>> -	int orig_count, new_count, status = 0, i;
>>> +	int ret, i;
>>> +	int status;
>>> +	int children;
>>> +	int level = 0;
>>>   	pid_t pids[MAX_DEPTH];
>>>   	pid_t pids_new[MAX_DEPTH];
>>>   -	orig_count = find_cinit_pids(pids);
>>> -	kill(pids[MAX_DEPTH - 3], SIGKILL);
>>> -	sleep(1);
>>> -
>>> -	/* After killing child container, getting the New PID list */
>>> -	new_count = find_cinit_pids(pids_new);
>>> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, &level);
>>> +	if (ret < 0)
>>> +		tst_brk(TBROK | TERRNO, "clone failed");
>>>   -	/* Verifying that the child containers were destroyed when
>>> parent is killed */
>>> -	if (orig_count - 2 != new_count)
>>> -		status = -1;
>>> +	TST_CHECKPOINT_WAIT(0);
>>>   -	for (i = 0; i < new_count; i++) {
>>> -		if (pids[i] != pids_new[i])
>>> -			status = -1;
>>> -	}
>>> +	find_cinit_pids(pids);
>>>   -	if (status == 0)
>>> -		tst_resm(TPASS, "The number of containers killed are %d",
>>> -			 orig_count - new_count);
>>> -	else
>>> -		tst_resm(TFAIL, "Failed to kill the sub-containers of "
>>> -			 "the container %d", pids[MAX_DEPTH - 3]);
>>> -
>>> -	/* Loops through the containers created to exit from sleep() */
>>> -	for (i = 0; i < MAX_DEPTH; i++) {
>>> -		kill(pids[i], SIGKILL);
>>> -		waitpid(pids[i], &status, 0);
>>> -	}
>>> -}
>>> +	SAFE_KILL(pids[0], SIGKILL);
>>>   -static void setup(void)
>>> -{
>>> -	tst_require_root();
>>> -	check_newpid();
>>> -}
>>> +	TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);
>>>   -int main(void)
>>> -{
>>> -	int ret, nbytes, status;
>>> -	char readbuffer[80];
>>> -	pid_t pid, pgid;
>>> -	int count = MAX_DEPTH;
>>> +	children = find_cinit_pids(pids_new);
>>>   -	setup();
>>> +	if (children > 0) {
>>> +		tst_res(TFAIL, "%d children left after sending SIGKILL", children);
>>>   -	/*
>>> -	 * XXX (garrcoop): why in the hell is this fork-wait written this way?
>>> -	 * This doesn't add up with the pattern used for the rest of the tests,
>>> -	 * so I'm pretty damn sure this test is written incorrectly.
>>> -	 */
>>> -	pid = fork();
>>> -	if (pid == -1) {
>>> -		tst_brkm(TBROK | TERRNO, NULL, "fork failed");
>>> -	} else if (pid != 0) {
>>> -		/*
>>> -		 * NOTE: use waitpid so that we know we're waiting for the
>>> -		 * _top-level_ child instead of a spawned subcontainer.
>>> -		 *
>>> -		 * XXX (garrcoop): Might want to mask SIGCHLD in the top-level
>>> -		 * child too, or not *shrugs*.
>>> -		 */
>>> -		if (waitpid(pid, &status, 0) == -1) {
>>> -			perror("wait failed");
>>> +		for (i = 0; i < MAX_DEPTH; i++) {
>>> +			kill(pids[i], SIGKILL);
>>> +			waitpid(pids[i], &status, 0);
>>>   		}
>>> -		if (WIFEXITED(status))
>>> -			exit(WEXITSTATUS(status));
>>> -		else
>>> -			exit(status);
>>> -	}
>>>   -	/* To make all the containers share the same PGID as its
>>> parent */
>>> -	setpgid(0, 0);
>>> -
>>> -	pid = getpid();
>>> -	pgid = getpgid(pid);
>>> -	SAFE_PIPE(NULL, fd);
>>> -
>>> -	TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>>> -				   create_nested_container, (void *)&count));
>>> -	if (TEST_RETURN == -1) {
>>> -		tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
>>> +		return;
>>>   	}
>>>   -	close(fd[1]);
>>> -	/* Waiting for the MAX_DEPTH number of containers to be created */
>>> -	nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
>>> -	close(fd[0]);
>>> -	if (nbytes > 0)
>>> -		tst_resm(TINFO, " %d %s", MAX_DEPTH, readbuffer);
>>> -	else
>>> -		tst_brkm(TFAIL, NULL, "unable to create %d containers",
>>> -			 MAX_DEPTH);
>>> -
>>> -	/* Kill the container created */
>>> -	kill_nested_containers();
>>> -
>>> -	tst_exit();
>>> +	tst_res(TPASS, "No children left after sending SIGKILL to the first child");
>>>   }
>>> +
>>> +static struct tst_test test = {
>>> +	.test_all = run,
>>> +	.setup = setup,
>>> +	.needs_root = 1,
>>> +	.needs_checkpoints = 1,
>>> +};
>>> -- 2.35.3
>>


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor pidns05 test using new LTP API

[Andrea Cervesato via ltp]

Hi,

On 2/7/23 11:51, Richard Palethorpe wrote:
> Hello,
>
> Andrea Cervesato <andrea.cervesato@suse.com> writes:
>
>> Hi!
>>
>> Can we merge this patch anyway? ltp_clone should be added after with a
>> single patch.
> I started work on an update to tst_clone so that it can replace most
> intances of ltp_clone. However it appears that it can already replace
> most instances of ltp_clone_quick.
>
> AFAICT all you need to do is get rid of child_func and use tst_clone
> like fork.
>
> I don't remember if there is anything else in this patch that needs
> reviewing, so there is no guarantee that when I look at this again it
> will get merged.
>
> So I don't see any advantage to deferring the change to tst_clone.

I sent a v2, please take a look at that one. I can start using tst_clone 
for the next reviews if needed.

>> Andrea
>>
>> On 10/11/22 11:56, Richard Palethorpe wrote:
>>> Hello,
>>>
>>> Andrea Cervesato via ltp <ltp@lists.linux.it> writes:
>>>
>>>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>>>> ---
>>>>    testcases/kernel/containers/pidns/pidns05.c | 288 ++++++--------------
>>>>    1 file changed, 78 insertions(+), 210 deletions(-)
>>>>
>>>> diff --git a/testcases/kernel/containers/pidns/pidns05.c b/testcases/kernel/containers/pidns/pidns05.c
>>>> index 79e146e36..1c588991b 100644
>>>> --- a/testcases/kernel/containers/pidns/pidns05.c
>>>> +++ b/testcases/kernel/containers/pidns/pidns05.c
>>>> @@ -1,256 +1,124 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>    /*
>>>> -* Copyright (c) International Business Machines Corp., 2007
>>>> -* 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
>>>> -*
>>>> -***************************************************************************
>>>> -*
>>>> -* Assertion:
>>>> -*   a) Create a  container.
>>>> -*   b) Create many levels of child containers inside this container.
>>>> -*   c) Now do kill -9 init , outside of the container.
>>>> -*   d) This should kill all the child containers.
>>>> -*      (containers created at the level below)
>>>> -*
>>>> -* Description:
>>>> -* 1. Parent process clone a process with flag CLONE_NEWPID
>>>> -* 2. The container will recursively loop and creates 4 more containers.
>>>> -* 3. All the container init's  goes into sleep(), waiting to be terminated.
>>>> -* 4. The parent process will kill child[3] by passing SIGKILL
>>>> -* 5. Now parent process, verifies the child containers 4 & 5 are destroyed.
>>>> -* 6. If they are killed then
>>>> -*	Test passed
>>>> -*  else Test failed.
>>>> -*
>>>> -* Test Name: pidns05
>>>> -*
>>>> -* History:
>>>> -*
>>>> -* FLAG DATE		NAME				DESCRIPTION
>>>> -* 31/10/08  Veerendra C <vechandr@in.ibm.com>	Verifies killing of NestedCont's
>>>> -*
>>>> -*******************************************************************************/
>>>> -#define _GNU_SOURCE 1
>>>> + * Copyright (c) International Business Machines Corp., 2007
>>>> + *		08/10/08 Veerendra C <vechandr@in.ibm.com>
>>>> + * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
>>>> + */
>>>> +
>>>> +/*\
>>>> + * [Description]
>>>> + *
>>>> + * Clone a process with CLONE_NEWPID flag and create many levels of child
>>>> + * containers. Then kill container init process from parent and check if all
>>>> + * containers have been killed.
>>>> + */
>>>> +
>>>>    #include <sys/wait.h>
>>>> -#include <assert.h>
>>>> -#include <stdio.h>
>>>> -#include <stdlib.h>
>>>> -#include <unistd.h>
>>>> -#include <string.h>
>>>> -#include <errno.h>
>>>> -#include "pidns_helper.h"
>>>> -#include "test.h"
>>>> -#include "safe_macros.h"
>>>> +#include "tst_test.h"
>>>> +#include "lapi/namespaces_constants.h"
>>>>    -#define INIT_PID	1
>>>> -#define CINIT_PID	1
>>>> -#define PARENT_PID	0
>>>>    #define MAX_DEPTH	5
>>>>    -char *TCID = "pidns05";
>>>> -int TST_TOTAL = 1;
>>>> -int fd[2];
>>>> +static pid_t pid_max;
>>>>    -int max_pid(void)
>>>> +static int child_func(void *arg)
>>>>    {
>>>> -	FILE *fp;
>>>>    	int ret;
>>>> +	int *level;
>>>> +	pid_t cpid, ppid;
>>>> +
>>>> +	cpid = getpid();
>>>> +	ppid = getppid();
>>>>    -	fp = fopen("/proc/sys/kernel/pid_max", "r");
>>>> -	if (fp != NULL) {
>>>> -		fscanf(fp, "%d", &ret);
>>>> -		fclose(fp);
>>>> -	} else {
>>>> -		tst_resm(TBROK, "Cannot open /proc/sys/kernel/pid_max");
>>>> -		ret = -1;
>>>> +	if (cpid != 1 || ppid != 0) {
>>>> +		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
>>>> +		return 1;
>>>>    	}
>>>> -	return ret;
>>>> +
>>>> +	level = (int *)arg;
>>>> +
>>>> +	if (*level >= MAX_DEPTH) {
>>>> +		TST_CHECKPOINT_WAKE(0);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	(*level)++;
>>>> +
>>>> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func,
>>>> level);
>>> Again, ltp_clone should be converted to tst_clone.
>>>
>>>> +	if (ret < 0)
>>>> +		tst_brk(TBROK | TERRNO, "clone failed");
>>>> +
>>>> +	pause();
>>>> +
>>>> +	return 0;
>>>>    }
>>>>    -/* find_cinit_pids() iteratively finds the pid's having same
>>>> PGID as its parent.
>>>> - * Input parameter - Accepts pointer to pid_t : To copy the pid's matching.
>>>> - * Returns - the number of pids matched.
>>>> -*/
>>>> -int find_cinit_pids(pid_t * pids)
>>>> +static int find_cinit_pids(pid_t *pids)
>>>>    {
>>>> -	int next = 0, pid_max, i;
>>>> +	int i;
>>>> +	int next = 0;
>>>>    	pid_t parentpid, pgid, pgid2;
>>>>    -	pid_max = max_pid();
>>>>    	parentpid = getpid();
>>>>    	pgid = getpgid(parentpid);
>>>>    -	/* The loop breaks, when the loop counter reaches the
>>>> parentpid value */
>>>> -	for (i = parentpid + 1; i != parentpid; i++) {
>>>> -		if (i > pid_max)
>>>> -			i = 2;
>>>> -
>>>> +	for (i = parentpid + 1; i < pid_max; i++) {
>>>>    		pgid2 = getpgid(i);
>>>> +
>>>>    		if (pgid2 == pgid) {
>>>>    			pids[next] = i;
>>>>    			next++;
>>>>    		}
>>>>    	}
>>>> +
>>>>    	return next;
>>>>    }
>>>>    -/*
>>>> -* create_nested_container() Recursively create MAX_DEPTH nested containers
>>>> -*/
>>>> -int create_nested_container(void *vtest)
>>>> +static void setup(void)
>>>>    {
>>>> -	int exit_val;
>>>> -	int ret, count, *level;
>>>> -	pid_t cpid, ppid;
>>>> -	cpid = getpid();
>>>> -	ppid = getppid();
>>>> -	char mesg[] = "Nested Containers are created";
>>>> -
>>>> -	level = (int *)vtest;
>>>> -	count = *level;
>>>> -
>>>> -	/* Child process closes up read side of pipe */
>>>> -	close(fd[0]);
>>>> -
>>>> -	/* Comparing the values to make sure pidns is created correctly */
>>>> -	if (cpid != CINIT_PID || ppid != PARENT_PID) {
>>>> -		printf("Got unexpected cpid and/or ppid (cpid=%d ppid=%d)\n",
>>>> -		       cpid, ppid);
>>>> -		exit_val = 1;
>>>> -	}
>>>> -	if (count > 1) {
>>>> -		count--;
>>>> -		ret = do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>>>> -					    create_nested_container,
>>>> -					    (void *)&count);
>>>> -		if (ret == -1) {
>>>> -			printf("clone failed; errno = %d : %s\n",
>>>> -			       ret, strerror(ret));
>>>> -			exit_val = 1;
>>>> -		} else
>>>> -			exit_val = 0;
>>>> -	} else {
>>>> -		/* Sending mesg, 'Nested containers created' through the pipe */
>>>> -		write(fd[1], mesg, (strlen(mesg) + 1));
>>>> -		exit_val = 0;
>>>> -	}
>>>> -
>>>> -	close(fd[1]);
>>>> -	pause();
>>>> -
>>>> -	return exit_val;
>>>> +	SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &pid_max);
>>>>    }
>>>>    -void kill_nested_containers()
>>>> +static void run(void)
>>>>    {
>>>> -	int orig_count, new_count, status = 0, i;
>>>> +	int ret, i;
>>>> +	int status;
>>>> +	int children;
>>>> +	int level = 0;
>>>>    	pid_t pids[MAX_DEPTH];
>>>>    	pid_t pids_new[MAX_DEPTH];
>>>>    -	orig_count = find_cinit_pids(pids);
>>>> -	kill(pids[MAX_DEPTH - 3], SIGKILL);
>>>> -	sleep(1);
>>>> -
>>>> -	/* After killing child container, getting the New PID list */
>>>> -	new_count = find_cinit_pids(pids_new);
>>>> +	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, &level);
>>>> +	if (ret < 0)
>>>> +		tst_brk(TBROK | TERRNO, "clone failed");
>>>>    -	/* Verifying that the child containers were destroyed when
>>>> parent is killed */
>>>> -	if (orig_count - 2 != new_count)
>>>> -		status = -1;
>>>> +	TST_CHECKPOINT_WAIT(0);
>>>>    -	for (i = 0; i < new_count; i++) {
>>>> -		if (pids[i] != pids_new[i])
>>>> -			status = -1;
>>>> -	}
>>>> +	find_cinit_pids(pids);
>>>>    -	if (status == 0)
>>>> -		tst_resm(TPASS, "The number of containers killed are %d",
>>>> -			 orig_count - new_count);
>>>> -	else
>>>> -		tst_resm(TFAIL, "Failed to kill the sub-containers of "
>>>> -			 "the container %d", pids[MAX_DEPTH - 3]);
>>>> -
>>>> -	/* Loops through the containers created to exit from sleep() */
>>>> -	for (i = 0; i < MAX_DEPTH; i++) {
>>>> -		kill(pids[i], SIGKILL);
>>>> -		waitpid(pids[i], &status, 0);
>>>> -	}
>>>> -}
>>>> +	SAFE_KILL(pids[0], SIGKILL);
>>>>    -static void setup(void)
>>>> -{
>>>> -	tst_require_root();
>>>> -	check_newpid();
>>>> -}
>>>> +	TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);
>>>>    -int main(void)
>>>> -{
>>>> -	int ret, nbytes, status;
>>>> -	char readbuffer[80];
>>>> -	pid_t pid, pgid;
>>>> -	int count = MAX_DEPTH;
>>>> +	children = find_cinit_pids(pids_new);
>>>>    -	setup();
>>>> +	if (children > 0) {
>>>> +		tst_res(TFAIL, "%d children left after sending SIGKILL", children);
>>>>    -	/*
>>>> -	 * XXX (garrcoop): why in the hell is this fork-wait written this way?
>>>> -	 * This doesn't add up with the pattern used for the rest of the tests,
>>>> -	 * so I'm pretty damn sure this test is written incorrectly.
>>>> -	 */
>>>> -	pid = fork();
>>>> -	if (pid == -1) {
>>>> -		tst_brkm(TBROK | TERRNO, NULL, "fork failed");
>>>> -	} else if (pid != 0) {
>>>> -		/*
>>>> -		 * NOTE: use waitpid so that we know we're waiting for the
>>>> -		 * _top-level_ child instead of a spawned subcontainer.
>>>> -		 *
>>>> -		 * XXX (garrcoop): Might want to mask SIGCHLD in the top-level
>>>> -		 * child too, or not *shrugs*.
>>>> -		 */
>>>> -		if (waitpid(pid, &status, 0) == -1) {
>>>> -			perror("wait failed");
>>>> +		for (i = 0; i < MAX_DEPTH; i++) {
>>>> +			kill(pids[i], SIGKILL);
>>>> +			waitpid(pids[i], &status, 0);
>>>>    		}
>>>> -		if (WIFEXITED(status))
>>>> -			exit(WEXITSTATUS(status));
>>>> -		else
>>>> -			exit(status);
>>>> -	}
>>>>    -	/* To make all the containers share the same PGID as its
>>>> parent */
>>>> -	setpgid(0, 0);
>>>> -
>>>> -	pid = getpid();
>>>> -	pgid = getpgid(pid);
>>>> -	SAFE_PIPE(NULL, fd);
>>>> -
>>>> -	TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWPID,
>>>> -				   create_nested_container, (void *)&count));
>>>> -	if (TEST_RETURN == -1) {
>>>> -		tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
>>>> +		return;
>>>>    	}
>>>>    -	close(fd[1]);
>>>> -	/* Waiting for the MAX_DEPTH number of containers to be created */
>>>> -	nbytes = read(fd[0], readbuffer, sizeof(readbuffer));
>>>> -	close(fd[0]);
>>>> -	if (nbytes > 0)
>>>> -		tst_resm(TINFO, " %d %s", MAX_DEPTH, readbuffer);
>>>> -	else
>>>> -		tst_brkm(TFAIL, NULL, "unable to create %d containers",
>>>> -			 MAX_DEPTH);
>>>> -
>>>> -	/* Kill the container created */
>>>> -	kill_nested_containers();
>>>> -
>>>> -	tst_exit();
>>>> +	tst_res(TPASS, "No children left after sending SIGKILL to the first child");
>>>>    }
>>>> +
>>>> +static struct tst_test test = {
>>>> +	.test_all = run,
>>>> +	.setup = setup,
>>>> +	.needs_root = 1,
>>>> +	.needs_checkpoints = 1,
>>>> +};
>>>> -- 2.35.3
>
Andrea Cervesato


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp