[LTP] [PATCH 2/3] syscalls/setns: convert to new API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
>  testcases/kernel/syscalls/setns/setns.h   |   8 +-
>  testcases/kernel/syscalls/setns/setns01.c |  78 +++++--------
>  testcases/kernel/syscalls/setns/setns02.c | 128 ++++++++++------------
>  3 files changed, 91 insertions(+), 123 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/setns/setns.h b/testcases/kernel/syscalls/setns/setns.h
> index a05e6736c64a..942d0833b782 100644
> --- a/testcases/kernel/syscalls/setns/setns.h
> +++ b/testcases/kernel/syscalls/setns/setns.h
> @@ -22,6 +22,8 @@
>   * 02110-1301, USA.
>   */
>  
> +#include <stdio.h>
> +
>  #define NS_MAX 5
>  static int ns_types[NS_MAX];
>  static int ns_fds[NS_MAX];
> @@ -37,10 +39,10 @@ static int get_ns_fd(int pid, const char *ns)
>  	if (stat(tmp, &st) == 0) {
>  		fd = open(tmp, O_RDONLY);
>  		if (fd == -1)
> -			tst_brkm(TBROK|TERRNO, NULL, "failed to open %s", tmp);
> +			tst_brk(TBROK|TERRNO, "failed to open %s", tmp);
>  	} else {
>  		if (errno != ENOENT)
> -			tst_brkm(TBROK|TERRNO, NULL, "failed to stat %s", tmp);
> +			tst_brk(TBROK|TERRNO, "failed to stat %s", tmp);
>  	}
>  	return fd;
>  }
> @@ -53,7 +55,7 @@ static void init_ns_type(int clone_type, const char *proc_name)
>  	if (fd != -1) {
>  		ns_types[ns_total] = clone_type;
>  		ns_fds[ns_total] = fd;
> -		tst_resm(TINFO, "ns_name=%s, ns_fds[%d]=%d, ns_types[%d]=0x%x",
> +		tst_res(TINFO, "ns_name=%s, ns_fds[%d]=%d, ns_types[%d]=0x%x",
>  			 proc_name, ns_total, fd, ns_total, clone_type);
>  		ns_total++;
>  	}

I guess that we can as well include the lapi/namespace_constants.h and
remove the #ifdefs in the init_available_ns() function.

> diff --git a/testcases/kernel/syscalls/setns/setns01.c b/testcases/kernel/syscalls/setns/setns01.c
> index 864899205aa8..0cf2684ba6ef 100644
> --- a/testcases/kernel/syscalls/setns/setns01.c
> +++ b/testcases/kernel/syscalls/setns/setns01.c
> @@ -26,20 +26,14 @@
>   */

Can we please change the GPL comment to SPDX as well?

>  #define _GNU_SOURCE
>  #include <sys/stat.h>
> -#include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <errno.h>
>  #include <sched.h>
>  #include <pwd.h>
>  #include <string.h>
>  #include "config.h"
> -#include "test.h"
> +#include "tst_test.h"
>  #include "lapi/syscalls.h"
> -#include "safe_macros.h"
> -
> -char *TCID = "setns01";
> -
> -#if defined(__NR_setns)
>  #include "setns.h"
>  
>  struct testcase_t {
> @@ -53,8 +47,6 @@ struct testcase_t {
>  	void (*cleanup) (struct testcase_t *);
>  };
>  
> -static void setup(void);
> -static void cleanup(void);
>  static void setup0(struct testcase_t *, int);
>  static void setup1(struct testcase_t *, int);
>  static void setup2(struct testcase_t *, int);
> @@ -63,7 +55,7 @@ static void setup4(struct testcase_t *, int);
>  static void cleanup1(struct testcase_t *);
>  static void cleanup4(struct testcase_t *);
>  
> -struct testcase_t tdat[] = {
> +struct testcase_t tcases[] = {
>  	{
>  		.msg = "invalid fd",
>  		.fd = -1,
> @@ -100,7 +92,6 @@ struct testcase_t tdat[] = {
>  	}
>  };
>  
> -static int TST_TOTAL = sizeof(tdat) / sizeof(tdat[0]);
>  static const char nobody_uid[] = "nobody";
>  static struct passwd *ltpuser;
>  
> @@ -114,7 +105,7 @@ static void setup1(struct testcase_t *t, int i)
>  	t->ns_type = ns_types[i];
>  	t->fd = open("dummy", O_RDWR|O_CREAT, 0600);
>  	if (t->fd == -1)
> -		tst_brkm(TFAIL|TERRNO, cleanup, "setup1:open failed");
> +		tst_brk(TFAIL|TERRNO, "setup1:open failed");
>  	unlink("dummy");
>  }
>  
> @@ -141,41 +132,42 @@ static void setup3(struct testcase_t *t, int i)
>  
>  static void setup4(struct testcase_t *t, int i)
>  {
> -	SAFE_SETEUID(NULL, ltpuser->pw_uid);
> +	SAFE_SETEUID(ltpuser->pw_uid);
>  
>  	t->fd = ns_fds[i];
>  	t->ns_type = ns_types[i];
>  }
>  
> -static void cleanup4(struct testcase_t *t)
> +static void cleanup4(LTP_ATTRIBUTE_UNUSED struct testcase_t *t)
>  {
> -	SAFE_SETEUID(NULL, 0);
> +	SAFE_SETEUID(0);
>  }

I would move these functions before the testcsese_t structure so that we
do not have to define the function prototypes.

Also I guess that we can open the regular file once in the test setup
and set only the t->fd in the setup1()

> -static void test_setns(struct testcase_t *t)
> +static void test_setns(unsigned int testno)
>  {
>  	int ret, i;
> +	struct testcase_t *t = &tcases[testno];
>  
>  	for (i = 0; i < ns_total; i++) {
>  		if (t->setup)
>  			t->setup(t, i);
>  
>  		if (t->skip) {
> -			tst_resm(TINFO, "skip %s", tdat->msg);
> +			tst_res(TINFO, "skip %s", t->msg);
>  			continue;
>  		}
>  
> -		tst_resm(TINFO, "setns(%d, 0x%x)", t->fd, t->ns_type);
> -		ret = syscall(__NR_setns, t->fd, t->ns_type);
> +		tst_res(TINFO, "setns(%d, 0x%x)", t->fd, t->ns_type);
> +		ret = tst_syscall(__NR_setns, t->fd, t->ns_type);
>  		if (ret == t->exp_ret) {
>  			if (ret == -1 && errno == t->exp_errno)
> -				tst_resm(TPASS, "%s exp_errno=%d", t->msg,
> +				tst_res(TPASS, "%s exp_errno=%d", t->msg,
>  						t->exp_errno);
>  			else
> -				tst_resm(TFAIL|TERRNO, "%s exp_errno=%d",
> +				tst_res(TFAIL|TERRNO, "%s exp_errno=%d",
>  					t->msg, t->exp_errno);
>  		} else {
> -			tst_resm(TFAIL, "%s ret=%d expected=%d", t->msg,
> +			tst_res(TFAIL, "%s ret=%d expected=%d", t->msg,
>  					ret, t->exp_ret);

Can we also print the expected errno name with tst_strerrno() ?

>  		}
>  
> @@ -184,50 +176,32 @@ static void test_setns(struct testcase_t *t)
>  	}
>  }
>  
> -int main(int argc, char *argv[])
> -{
> -	int lc, testno;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		for (testno = 0; testno < TST_TOTAL; testno++)
> -			test_setns(&tdat[testno]);
> -	}
> -	cleanup();
> -	tst_exit();
> -}
> -
>  static void setup(void)
>  {
> -	tst_require_root();
> -
>  	/* runtime check if syscall is supported */
> -	ltp_syscall(__NR_setns, -1, 0);
> +	tst_syscall(__NR_setns, -1, 0);
>  
>  	init_available_ns();
>  	if (ns_total == 0)
> -		tst_brkm(TCONF, NULL, "no ns types/proc entries");
> +		tst_brk(TCONF, "no ns types/proc entries");
>  
>  	ltpuser = getpwnam(nobody_uid);
>  	if (ltpuser == NULL)
> -		tst_brkm(TBROK | TERRNO, NULL, "getpwnam failed");
> +		tst_brk(TBROK | TERRNO, "getpwnam failed");
>  
> -
> -	tst_tmpdir();
> -	TEST_PAUSE;
>  }
>  
>  static void cleanup(void)
>  {
>  	close_ns_fds();
> -	tst_rmdir();
>  }
> -#else
> -int main(int argc, char *argv[])
> -{
> -	tst_brkm(TCONF, NULL, "__NR_setns is not defined on your system.");
>  
> -}
> -#endif
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = test_setns,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +};
> +
> diff --git a/testcases/kernel/syscalls/setns/setns02.c b/testcases/kernel/syscalls/setns/setns02.c
> index 0f49028450b7..ce97be807256 100644
> --- a/testcases/kernel/syscalls/setns/setns02.c
> +++ b/testcases/kernel/syscalls/setns/setns02.c
> @@ -33,24 +33,20 @@
>  #include <sys/ipc.h>
>  #include <sys/shm.h>
>  #include <sys/stat.h>
> -#include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <sys/utsname.h>
>  #include <sys/wait.h>
>  #include <errno.h>
>  #include <sched.h>
> +#include <stdlib.h>
>  #include <string.h>
>  #include "config.h"
> -#include "test.h"
> +#include "tst_test.h"
>  #include "lapi/syscalls.h"
> -#include "safe_macros.h"
> +#include "setns.h"
>  
>  #define CHILD_STACK_SIZE (1024*1024)
>  #define CP "(child) "
> -char *TCID = "setns02";
> -
> -#if defined(__NR_setns) && defined(CLONE_NEWIPC) && defined(CLONE_NEWUTS)
> -#include "setns.h"
>  
>  static char *dummy_hostname = "setns_dummy_uts";
>  static int ns_ipc_fd;
> @@ -67,37 +63,37 @@ static int do_child_newuts(void *arg)
>  	int ns_flag = *(int *)arg;
>  
>  	if (uname(&uts_parent) == -1)
> -		tst_resm(TFAIL|TERRNO, CP"uname");
> -	tst_resm(TINFO, CP"hostname (inherited from parent): %s",
> +		tst_res(TFAIL|TERRNO, CP"uname");
> +	tst_res(TINFO, CP"hostname (inherited from parent): %s",
>  		uts_parent.nodename);
>  
>  	if (sethostname(dummy_hostname, strlen(dummy_hostname)) == -1)
> -		tst_resm(TFAIL|TERRNO, CP"sethostname");
> +		tst_res(TFAIL|TERRNO, CP"sethostname");
>  	if (uname(&uts) == -1)
> -		tst_resm(TFAIL|TERRNO, CP"uname");
> +		tst_res(TFAIL|TERRNO, CP"uname");
>  
> -	tst_resm(TINFO, CP"hostname changed to: %s", uts.nodename);
> +	tst_res(TINFO, CP"hostname changed to: %s", uts.nodename);
>  	if (strcmp(uts_parent.nodename, uts.nodename) == 0) {
> -		tst_resm(TFAIL, CP"expected hostname to be different");
> +		tst_res(TFAIL, CP"expected hostname to be different");
>  		return 1;
>  	} else {
> -		tst_resm(TPASS, CP"hostname is different in parent/child");
> +		tst_res(TPASS, CP"hostname is different in parent/child");
>  	}
>  
> -	tst_resm(TINFO, CP"attempting to switch ns back to parent ns");
> -	if (syscall(__NR_setns, ns_uts_fd, ns_flag) == -1) {
> -		tst_resm(TFAIL|TERRNO, CP"setns");
> +	tst_res(TINFO, CP"attempting to switch ns back to parent ns");
> +	if (tst_syscall(__NR_setns, ns_uts_fd, ns_flag) == -1) {
> +		tst_res(TFAIL|TERRNO, CP"setns");
>  		return 2;
>  	}
>  	if (uname(&uts) == -1)
> -		tst_resm(TFAIL|TERRNO, CP"uname");
> +		tst_res(TFAIL|TERRNO, CP"uname");
>  
> -	tst_resm(TINFO, CP"hostname: %s", uts.nodename);
> +	tst_res(TINFO, CP"hostname: %s", uts.nodename);
>  	if (strcmp(uts_parent.nodename, uts.nodename) != 0) {
> -		tst_resm(TFAIL, CP"expected hostname to match parent");
> +		tst_res(TFAIL, CP"expected hostname to match parent");
>  		return 3;
>  	} else {
> -		tst_resm(TPASS, CP"hostname now as expected");
> +		tst_res(TPASS, CP"hostname now as expected");
>  	}
>  	return 0;
>  }
> @@ -109,25 +105,25 @@ static int do_child_newipc(void *arg)
>  
>  	p = shmat(shmid, NULL, 0);
>  	if (p == (void *) -1) {
> -		tst_resm(TPASS|TERRNO, CP"shmat failed as expected");
> +		tst_res(TPASS|TERRNO, CP"shmat failed as expected");
>  	} else {
> -		tst_resm(TFAIL, CP"shmat unexpectedly suceeded");
> +		tst_res(TFAIL, CP"shmat unexpectedly suceeded");
>  		shmdt(p);
>  		return 1;
>  	}
>  
> -	tst_resm(TINFO, CP"attempting to switch ns back to parent ns");
> -	if (syscall(__NR_setns, ns_ipc_fd, ns_flag) == -1) {
> -		tst_resm(TFAIL|TERRNO, CP"setns");
> +	tst_res(TINFO, CP"attempting to switch ns back to parent ns");
> +	if (tst_syscall(__NR_setns, ns_ipc_fd, ns_flag) == -1) {
> +		tst_res(TFAIL|TERRNO, CP"setns");
>  		return 2;
>  	}
>  
>  	p = shmat(shmid, NULL, 0);
>  	if (p == (void *) -1) {
> -		tst_resm(TFAIL|TERRNO, CP"shmat failed after setns");
> +		tst_res(TFAIL|TERRNO, CP"shmat failed after setns");
>  		return 3;
>  	} else {
> -		tst_resm(TPASS, CP"shmat suceeded");
> +		tst_res(TPASS, CP"shmat suceeded");
>  		shmdt(p);
>  	}
>  
> @@ -141,70 +137,66 @@ static void test_flag(int clone_flag, int ns_flag, int (*fn) (void *arg))
>  
>  	child_stack = malloc(CHILD_STACK_SIZE);
>  	if (child_stack == NULL)
> -		tst_brkm(TBROK, cleanup, "Cannot allocate stack for child");
> +		tst_brk(TBROK, "Cannot allocate stack for child");
>  
> -	tst_resm(TINFO, "creating child with clone_flag=0x%x, ns_flag=0x%x",
> +	tst_res(TINFO, "creating child with clone_flag=0x%x, ns_flag=0x%x",
>  		clone_flag, ns_flag);
>  	ret = ltp_clone(SIGCHLD|clone_flag, fn, &ns_flag,
>  		CHILD_STACK_SIZE, child_stack);
>  	if (ret == -1)
> -		tst_brkm(TBROK|TERRNO, cleanup, "ltp_clone");
> +		tst_brk(TBROK|TERRNO, "ltp_clone");
>  
> -	SAFE_WAITPID(cleanup, ret, &status, 0);
> +	SAFE_WAITPID(ret, &status, 0);
>  	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> -		tst_resm(TFAIL, "child returns %d", status);
> +		tst_res(TFAIL, "child returns %d", status);
>  	else
> -		tst_resm(TPASS, "child finished succesfully");
> +		tst_res(TPASS, "child finished succesfully");
>  	free(child_stack);
>  }

Do we still need to propagate the test result via exit value?

That shouldn't be needed in new API test, right?

> -int main(int argc, char *argv[])
> +void test_all(void)
>  {
> -	int lc;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		if (ns_uts_fd != -1) {
> -			tst_resm(TINFO, "test_newuts");
> -			test_flag(CLONE_NEWUTS, CLONE_NEWUTS, do_child_newuts);
> -			test_flag(CLONE_NEWUTS, 0, do_child_newuts);
> -		}
> -		if (ns_ipc_fd != -1) {
> -			tst_resm(TINFO, "test_newipc");
> -			test_flag(CLONE_NEWIPC, CLONE_NEWIPC, do_child_newipc);
> -			test_flag(CLONE_NEWIPC, 0, do_child_newipc);
> -		}
> +	int report_tconf = 1;
> +#if defined(CLONE_NEWUTS)
> +	if (ns_uts_fd != -1) {
> +		tst_res(TINFO, "test_newuts");
> +		test_flag(CLONE_NEWUTS, CLONE_NEWUTS, do_child_newuts);
> +		test_flag(CLONE_NEWUTS, 0, do_child_newuts);
>  	}
> -	cleanup();
> -	tst_exit();
> +	report_tconf = 0;
> +#endif
> +#if defined(CLONE_NEWIPC)
> +	if (ns_ipc_fd != -1) {
> +		tst_res(TINFO, "test_newipc");
> +		test_flag(CLONE_NEWIPC, CLONE_NEWIPC, do_child_newipc);
> +		test_flag(CLONE_NEWIPC, 0, do_child_newipc);
> +	}
> +	report_tconf = 0;
> +#endif
> +	if (report_tconf)
> +		tst_brk(TCONF, "CLONE_NEWIPC and CLONE_NEWUTS is not defined");

Can we get rid of these ifdefs as well? If we include the
lapi/namespace_constants.h in the setns.h we can just remove these,
right?

>  }
>  
>  static void setup(void)
>  {
>  	char tmp[PATH_MAX];
>  
> -	tst_require_root();
> -
>  	/* runtime check if syscall is supported */
> -	ltp_syscall(__NR_setns, -1, 0);
> +	tst_syscall(__NR_setns, -1, 0);
>  
>  	/* check if kernel has CONFIG_*_NS set and exports /proc entries */
>  	ns_ipc_fd = get_ns_fd(getpid(), "ipc");
>  	ns_uts_fd = get_ns_fd(getpid(), "uts");
>  	if (ns_ipc_fd == -1 && ns_uts_fd == -1)
> -		tst_brkm(TCONF, NULL, "your kernel has CONFIG_IPC_NS, "
> +		tst_brk(TCONF, "your kernel has CONFIG_IPC_NS, "
>  			"CONFIG_UTS_NS or CONFIG_PROC disabled");
>  
>  	if (getcwd(tmp, PATH_MAX) == NULL)
> -		tst_brkm(TBROK|TERRNO, NULL, "getcwd");
> +		tst_brk(TBROK|TERRNO, "getcwd");
>  	ipc_key = ftok(tmp, 65);
>  	shmid = shmget(ipc_key, getpagesize(), IPC_CREAT | 0666);
>  	if (shmid == -1)
> -		tst_brkm(TBROK|TERRNO, NULL, "shmget");
> -
> -	TEST_PAUSE;
> +		tst_brk(TBROK|TERRNO, "shmget");
>  }
>  
>  static void cleanup(void)
> @@ -216,10 +208,10 @@ static void cleanup(void)
>  
>  	shmctl(shmid, IPC_RMID, NULL);
>  }
> -#else
> -int main(int argc, char *argv[])
> -{
> -	tst_brkm(TCONF, NULL, "__NR_setns, CLONE_NEWIPC or CLONE_NEWUTS "
> -		" is not defined on your system.");
> -}
> -#endif
> +
> +static struct tst_test test = {
> +	.test_all = test_all,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +};
> -- 
> 2.18.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz