Re: [LTP] [PATCH v1 3/9] Rewrite msg_comm.c using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
Similar here.

>  #define _GNU_SOURCE
> +
>  #include <sys/ipc.h>
> +#include <sys/wait.h>
>  #include <sys/msg.h>
>  #include <sys/types.h>
> -#include <sys/wait.h>
> -#include <stdio.h>
> -#include <errno.h>
> -#include "ipcns_helper.h"
> -#include "test.h"
> -#include "safe_macros.h"
> +#include "tst_test.h"
> +#include "common.h"
>  
>  #define TESTKEY 124426L
>  #define MSGSIZE 50
> -char *TCID	= "msg_comm";
> -int TST_TOTAL	= 1;
>  
>  struct sysv_msg {
>  	long mtype;
>  	char mtext[MSGSIZE];
>  };
>  
> -static void cleanup(void)
> +static int chld1_msg(LTP_ATTRIBUTE_UNUSED void *arg)
>  {
> -	tst_rmdir();
> -}
> -
> -static void setup(void)
> -{
> -	tst_require_root();
> -	check_newipc();
> -	tst_tmpdir();
> -	TST_CHECKPOINT_INIT(tst_rmdir);
> -}
> -
> -int chld1_msg(void *arg)
> -{
> -	int id, n, rval = 0;
> +	int id, ret, rval = 0;
>  	struct sysv_msg m;
>  	struct sysv_msg rec;
>  
>  	id = msgget(TESTKEY, IPC_CREAT | 0600);
> -	if (id == -1) {
> -		perror("msgget");
> -		return 2;
> -	}
> +	if (id < 0)
> +		tst_brk(TBROK, "msgget: %s", tst_strerrno(-id));
>  
>  	m.mtype = 1;
>  	m.mtext[0] = 'A';
> -	if (msgsnd(id, &m, sizeof(struct sysv_msg) - sizeof(long), 0) == -1) {
> -		perror("msgsnd");
> +
> +	ret = msgsnd(id, &m, sizeof(struct sysv_msg) - sizeof(long), 0);
> +	if (ret < 0) {
>  		msgctl(id, IPC_RMID, NULL);
> -		return 2;
> +		tst_brk(TBROK, "msgsnd: %s", tst_strerrno(-ret));
>  	}
>  
>  	/* wait for child2 to write into the message queue */
> -	TST_SAFE_CHECKPOINT_WAIT(NULL, 0);
> +	TST_CHECKPOINT_WAIT(0);
>  
>  	/* if child1 message queue has changed (by child2) report fail */
> -	n = msgrcv(id, &rec, sizeof(struct sysv_msg) - sizeof(long),
> -		   2, IPC_NOWAIT);
> -	if (n == -1 && errno != ENOMSG) {
> -		perror("msgrcv");
> +	ret = msgrcv(id, &rec, sizeof(struct sysv_msg) - sizeof(long), 2,
> +		     IPC_NOWAIT);
> +	if (ret < 0 && errno != ENOMSG) {
>  		msgctl(id, IPC_RMID, NULL);
> -		return 2;
> +		tst_brk(TBROK, "msgrcv: %s", tst_strerrno(-ret));

This is wrong in many places, the msg*() calls return -1 and set errno
so you really just need to pass TERRNO along with the TBROK.

>  	}
> +
>  	/* if mtype #2 was found in the message queue, it is fail */
> -	if (n > 0) {
> +	if (ret > 0)
>  		rval = 1;
> -	}
>  
>  	/* tell child2 to continue */
> -	TST_SAFE_CHECKPOINT_WAKE(NULL, 0);
> +	TST_CHECKPOINT_WAKE(0);
>  
>  	msgctl(id, IPC_RMID, NULL);
> +
>  	return rval;
>  }
>  
> -int chld2_msg(void *arg)
> +static int chld2_msg(LTP_ATTRIBUTE_UNUSED void *arg)
>  {
> -	int id;
> +	int id, ret;
>  	struct sysv_msg m;
>  
>  	id = msgget(TESTKEY, IPC_CREAT | 0600);
> -	if (id == -1) {
> -		perror("msgget");
> -		return 2;
> -	}
> +	if (id < 0)
> +		tst_brk(TBROK, "msgget: %s", tst_strerrno(-id));
>  
>  	m.mtype = 2;
>  	m.mtext[0] = 'B';
> -	if (msgsnd(id, &m, sizeof(struct sysv_msg) - sizeof(long), 0) == -1) {
> -		perror("msgsnd");
> +
> +	ret = msgsnd(id, &m, sizeof(struct sysv_msg) - sizeof(long), 0);
> +	if (ret < 0) {
>  		msgctl(id, IPC_RMID, NULL);
> -		return 2;
> +		tst_brk(TBROK, "msgsnd: %s", tst_strerrno(-ret));
>  	}
>  
>  	/* tell child1 to continue and wait for it */
> -	TST_SAFE_CHECKPOINT_WAKE_AND_WAIT(NULL, 0);
> +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
>  
>  	msgctl(id, IPC_RMID, NULL);
> +
>  	return 0;
>  }
>  
> -static void test(void)
> +static void run(void)
>  {
>  	int status, ret = 0;
>  
> -	ret = do_clone_unshare_test(T_CLONE, CLONE_NEWIPC, chld1_msg, NULL);
> -	if (ret == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "clone failed");
> -
> -	ret = do_clone_unshare_test(T_CLONE, CLONE_NEWIPC, chld2_msg, NULL);
> -	if (ret == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "clone failed");
> -
> +	clone_unshare_test(T_CLONE, CLONE_NEWIPC, chld1_msg, NULL);
> +	clone_unshare_test(T_CLONE, CLONE_NEWIPC, chld2_msg, NULL);
>  
>  	while (wait(&status) > 0) {
>  		if (WIFEXITED(status) && WEXITSTATUS(status) == 1)
>  			ret = 1;
> +
>  		if (WIFEXITED(status) && WEXITSTATUS(status) == 2)
> -			tst_brkm(TBROK | TERRNO, cleanup, "error in child");
> +			tst_brk(TBROK, "error in child");
> +
>  		if (WIFSIGNALED(status)) {
> -			tst_resm(TFAIL, "child was killed with signal %s",
> -					tst_strsig(WTERMSIG(status)));
> -			return;
> +			tst_brk(TBROK, "child was killed with signal %s",
> +				tst_strsig(WTERMSIG(status)));
>  		}
>  	}
>  
> -	if (ret)
> -		tst_resm(TFAIL, "SysV msg: communication with identical keys"
> -				" between namespaces");
> -	else
> -		tst_resm(TPASS, "SysV msg: communication with identical keys"
> -				" between namespaces");
> +	if (ret) {
> +		tst_res(TFAIL, "SysV msg: communication with identical keys"
> +			       " between namespaces");
> +	} else {
> +		tst_res(TPASS, "SysV msg: communication with identical keys"
> +			       " between namespaces");
> +	}

No need to propagate anything at all, just do the PASS/FAIL in the
respective tests.

>  }
>  
> -int main(int argc, char *argv[])
> +static void setup(void)
>  {
> -	int lc;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++)
> -		test();
> -
> -	cleanup();
> -	tst_exit();
> +	check_newipc();
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_root = 1,
> +	.needs_checkpoints = 1,
> +};
> -- 
> 2.35.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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