Re: [LTP] [PATCH v1 07/10] Refactor userns07 test

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

All merged, thanks!

This one I made some changes to, please see below.

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

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  testcases/kernel/containers/userns/userns07.c | 67 +++++++++++--------
>  1 file changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/testcases/kernel/containers/userns/userns07.c b/testcases/kernel/containers/userns/userns07.c
> index 4659de636..3a693b8e3 100644
> --- a/testcases/kernel/containers/userns/userns07.c
> +++ b/testcases/kernel/containers/userns/userns07.c
> @@ -16,80 +16,89 @@
>  #include <sys/wait.h>
>  #include "common.h"
>  #include "tst_test.h"
> +#include "lapi/sched.h"
>  
>  #define MAXNEST 32
>  
> -static void setup(void)
> -{
> -	check_newuser();
> -}
> +static int *level;

AFAICT we do not need this variable and to use mmap. We can just pass an
int to child_fn1.

Unless you wanted to check at the end of the test that level == 32?
However you weren't doing that, so I just removed it.

>  
> -static int child_fn1(void *arg)
> +static void child_fn1(void)
>  {
> -	pid_t cpid1;
> -	long level = (long)arg;
> -	int status;
> +	const struct tst_clone_args args = { CLONE_NEWUSER, SIGCHLD };
> +	pid_t cpid;
>  	int parentuid;
>  	int parentgid;
> +	int status;
>  
>  	TST_CHECKPOINT_WAIT(0);
>  
> -	if (level == MAXNEST) {
> +	if (*level == MAXNEST) {
>  		tst_res(TPASS, "nested all children");
> -		return 0;
> +		return;
>  	}
>  
> -	cpid1 = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, (void *)child_fn1, (void *)(level + 1));
> -	if (cpid1 < 0) {
> -		tst_res(TFAIL | TERRNO, "level %ld, unexpected error", level);
> -		return 1;
> +	cpid = SAFE_CLONE(&args);
> +	if (!cpid) {
> +		*level += 1;
> +		child_fn1();
> +		return;
>  	}
>  
>  	parentuid = geteuid();
>  	parentgid = getegid();
>  
> -	updatemap(cpid1, UID_MAP, 0, parentuid);
> -	updatemap(cpid1, GID_MAP, 0, parentgid);
> +	updatemap(cpid, UID_MAP, 0, parentuid);
> +	updatemap(cpid, GID_MAP, 0, parentgid);
>  
>  	TST_CHECKPOINT_WAKE(0);
>  
> -	SAFE_WAITPID(cpid1, &status, 0);
> -
> -	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> -		tst_brk(TBROK, "child %s", tst_strstatus(status));
> -
> -	return 0;
> +	SAFE_WAITPID(cpid, &status, 0);

replaced with tst_reap_children.

>  }
>  
>  static void run(void)
>  {
> -	pid_t cpid1;
> +	const struct tst_clone_args args = { CLONE_NEWUSER, SIGCHLD };
> +	pid_t cpid;
>  	int parentuid;
>  	int parentgid;
>  	char path[BUFSIZ];
>  
> -	cpid1 = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, (void *)child_fn1, (void *)0);
> -	if (cpid1 < 0)
> -		tst_brk(TBROK | TTERRNO, "clone failed");
> +	cpid = SAFE_CLONE(&args);
> +	if (!cpid) {
> +		child_fn1();
> +		return;
> +	}
>  
>  	parentuid = geteuid();
>  	parentgid = getegid();
>  
>  	if (access("/proc/self/setgroups", F_OK) == 0) {
> -		sprintf(path, "/proc/%d/setgroups", cpid1);
> +		sprintf(path, "/proc/%d/setgroups", cpid);
>  		SAFE_FILE_PRINTF(path, "deny");
>  	}
>  
> -	updatemap(cpid1, UID_MAP, 0, parentuid);
> -	updatemap(cpid1, GID_MAP, 0, parentgid);
> +	updatemap(cpid, UID_MAP, 0, parentuid);
> +	updatemap(cpid, GID_MAP, 0, parentgid);
>  
>  	TST_CHECKPOINT_WAKE(0);
>  }
>  
> +static void setup(void)
> +{
> +	level = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_MUNMAP(level, sizeof(int));
> +}
> +
>  static struct tst_test test = {
>  	.setup = setup,
> +	.cleanup = cleanup,
>  	.test_all = run,
>  	.needs_root = 1,
> +	.forks_child = 1,
>  	.needs_checkpoints = 1,
>  	.needs_kconfigs = (const char *[]) {
>  		"CONFIG_USER_NS",
> -- 
> 2.35.3


-- 
Thank you,
Richard.

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