Re: [LTP] [PATCH v1] Remove libclone dependency from pidns05 test

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +// SPDX-License-Identifier: GPL-2.0

The original test was GPL-2.0-or-later, please pay a bit more attention
on the licenses, it's tedious and boring but it helps us avoid a trouble
in the long term.

> -	/* 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);

I do not think that this change is correct. The difference is that if
parentpid is close to pid_max the child pids would "overflow".

I suppose the easiest solution is to scan everything from 2 to pid_max
instead of depending on signed overflow, which I think is undefined to
begin with.

>  		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;
> -	pid_t pids[MAX_DEPTH];
> +	int i, status, children;
> +	int level = 0;
>  	pid_t pids_new[MAX_DEPTH];
> +	pid_t pids[MAX_DEPTH];
> +	pid_t pid;
>  
> -	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);
> -
> -	/* Verifying that the child containers were destroyed when parent is killed */
> -	if (orig_count - 2 != new_count)
> -		status = -1;
> -
> -	for (i = 0; i < new_count; i++) {
> -		if (pids[i] != pids_new[i])
> -			status = -1;
> +	pid = SAFE_CLONE(&clone_args);
> +	if (!pid) {
> +		child_func(&level);
> +		return;
>  	}
>  
> -	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);
> -	}
> -}
> +	TST_CHECKPOINT_WAIT(0);
>  
> -static void setup(void)
> -{
> -	tst_require_root();
> -	check_newpid();
> -}
> +	find_cinit_pids(pids);
>  
> -int main(void)
> -{
> -	int ret, nbytes, status;
> -	char readbuffer[80];
> -	pid_t pid, pgid;
> -	int count = MAX_DEPTH;
> +	SAFE_KILL(pids[0], SIGKILL);

Why do we do the find_cinit_pids() here and kill(pids[0], ...) shouldn't
we just use the pid from the clone above?

> -	setup();
> +	TST_RETRY_FUNC(waitpid(0, &status, WNOHANG), TST_RETVAL_NOTNULL);

Why are we wrapping WNOHANG call to waitpid into RETRY_FUNC? Just do
SAFE_WAITPID() without the WNOHANG.

Are you trying to collect all children here? That will not work at all
with the retry func, as it will exit on first succesful waitpid.

The original code did waitpid() for the top level child only here if you
want to wait all of them, you have to loop with WNOHANG and you can
count them in the loop as well...

> -	/*
> -	 * 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");
> -		}
> -		if (WIFEXITED(status))
> -			exit(WEXITSTATUS(status));
> -		else
> -			exit(status);
> -	}
> +	children = find_cinit_pids(pids_new);
>  
> -	/* To make all the containers share the same PGID as its parent */
> -	setpgid(0, 0);
> +	if (children > 0) {
> +		tst_res(TFAIL, "%d children left after sending SIGKILL", children);
>  
> -	pid = getpid();
> -	pgid = getpgid(pid);
> -	SAFE_PIPE(NULL, fd);
> +		for (i = 0; i < MAX_DEPTH; i++) {
> +			kill(pids[i], SIGKILL);
> +			waitpid(pids[i], &status, 0);
> +		}
>  
> -	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,
> +	.forks_child = 1,
> +};

-- 
Cyril Hrubis
chrubis@suse.cz

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