[LTP] [PATCH 2/2] ipc/msgsnd0*: cleanup && convert to new API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +static void cleanup(void)
>  {
> -	/* if it exists, remove the message queue if it exists */
> -	rm_queue(msg_q_1);
> -
> -	tst_rmdir();
> -
> +	if (queue_id != -1 && msgctl(queue_id, IPC_RMID, NULL)) {

You should initialized the queue_id to -1, since otherwise you may
attempt to remove queue id 0 if cleanup is executed before msgget()
returns and queue_id is initialized.

> +		tst_res(TWARN | TERRNO, "failed to delete message queue %i",
> +			queue_id);
> +	}
>  }
> +
> +static struct tst_test test = {
> +	.tid = "msgsnd01",
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = verify_msgsnd,
> +	.needs_tmpdir = 1
> +}

Otherwise it looks fine.

> diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd02.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd02.c
> index 6547ab2..9367775 100644

...

> +static key_t msgkey;
> +static int queue_id = -1;
> +static int bad_id = -1;
> +static struct passwd *pw;
> +static struct buf {
> +	long type;
> +	char text[MSGSIZE];
> +} snd_buf = {1, "hello, world"};
> +
> +static struct tcase {
> +	int *id;
> +	struct buf *buffer;
> +	long mtype;
> +	int msgsz;
> +	int exp_err;
> +	/*1: nobody expected  0: root expected */
> +	int exp_user;
> +} tcases[] = {
> +	{&queue_id, &snd_buf, 1, MSGSIZE, EACCES, 1},
> +	{&queue_id, NULL, 1, MSGSIZE, EFAULT, 0},
> +	{&bad_id, &snd_buf, 1, MSGSIZE, EINVAL, 0},
> +	{&queue_id, &snd_buf, 0, MSGSIZE, EINVAL, 0},
> +	{&queue_id, &snd_buf, -1, MSGSIZE, EINVAL, 0},
> +	{&queue_id, &snd_buf, 1, -1, EINVAL, 0}
> +};
> +
> +static void verify_msgsnd(struct tcase *tc)
>  {
> -	int lc;
> -	int i;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();		/* global setup */
> -
> -	/* The following loop checks looping state if -i option given */
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		/* reset tst_count in case we are looping */
> -		tst_count = 0;
> -
> -		/*
> -		 * loop through the test cases
> -		 */
> +	snd_buf.type = tc->mtype;

This is kind of hack. Why can't we create three struct buf variables and
assign them to the tcases structure instead of modyfing it this way?


Otherwise it looks fine.

> diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd05.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd05.c
> index 16d62f8..0d13065 100644
> --- a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd05.c
> +++ b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd05.c
> @@ -1,234 +1,125 @@
>  /*
> + * Copyright (c) International Business Machines  Corp., 2001
>   *
> - *   Copyright (c) International Business Machines  Corp., 2001
> + * 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 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.
>   *
> - *   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
> + * You should have received a copy of the GNU General Public License
> + * along with this program.
>   */
>  
>  /*
> - * NAME
> - *	msgsnd05.c
> - *
>   * DESCRIPTION
> - *	msgsnd05 - test for EINTR error
> - *
> - * ALGORITHM
> - *	create a message queue with read/write permissions
> - *	initialize a message buffer with a known message and type
> - *	enqueue the message in a loop until the queue is full
> - *	loop if that option was specified
> - *	fork a child process
> - *	child attempts to enqueue a message to the full queue and sleeps
> - *	parent sends a SIGHUP to the child then waits for the child to complete
> - *	child get a return from msgsnd()
> - *	check the errno value
> - *	  issue a PASS message if we get EINTR
> - *	otherwise, the tests fails
> - *	  issue a FAIL message
> - *	child exits, parent calls cleanup
> - *
> - * USAGE:  <for command-line>
> - *  msgsnd05 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
> - *     where,  -c n : Run n copies concurrently.
> - *             -e   : Turn on errno logging.
> - *	       -i n : Execute test n times.
> - *	       -I x : Execute test for x seconds.
> - *	       -P x : Pause for x seconds between iterations.
> - *	       -t   : Turn on syscall timing.
> - *
> - * HISTORY
> - *	03/2001 - Written by Wayne Boyer
> - *      14/03/2008 Matthieu Fertr?? (Matthieu.Fertre@irisa.fr)
> - *      - Fix concurrency issue. Due to the use of usleep function to
> - *        synchronize processes, synchronization issues can occur on a loaded
> - *        system. Fix this by using pipes to synchronize processes.
> - *
> - * RESTRICTIONS
> - *	none
> + * 1) test for EAGAIN error
> + * 2) test for EINTR error
>   */
>  
> -#include "test.h"
> -
> -#include "ipcmsg.h"
> -
> +#include <errno.h>
> +#include <stdlib.h>
>  #include <sys/types.h>
> -#include <sys/wait.h>
> -
> -void do_child(void);
> -void cleanup(void);
> -void setup(void);
> -#ifdef UCLINUX
> -#define PIPE_NAME	"msgsnd05"
> -void do_child_uclinux(void);
> -#endif
> -
> -char *TCID = "msgsnd05";
> -int TST_TOTAL = 1;
> -
> -int msg_q_1 = -1;		/* The message queue id created in setup */
> -
> -MSGBUF msg_buf;
> -
> -int main(int ac, char **av)
> +#include <sys/ipc.h>
> +#include <sys/msg.h>
> +
> +#include "tst_test.h"
> +#include "libnewipc.h"
> +
> +static key_t msgkey;
> +static int queue_id = -1;
> +static struct buf {
> +	long type;
> +	char text[MSGSIZE];
> +} snd_buf = {1, "hello, world"};
> +
> +static struct tcase {
> +	int flag;
> +	int exp_err;
> +	/*1: nobody expected  0: root expected */
> +	int exp_user;
> +} tcases[] = {
> +	{IPC_NOWAIT, EAGAIN, 0},
> +	{0, EINTR, 1}
> +};
> +
> +static void verify_msgsnd(struct tcase *tc)
>  {
> -	int lc;
> -	pid_t c_pid;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -#ifdef UCLINUX
> -	maybe_run_child(&do_child_uclinux, "d", &msg_q_1);
> -#endif
> -
> -	setup();		/* global setup */
> -
> -	/* The following loop checks looping state if -i option given */
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		/* reset tst_count in case we are looping */
> -		tst_count = 0;
> -
> -		/*
> -		 * fork a child that will attempt to write a message
> -		 * to the queue without IPC_NOWAIT
> -		 */
> -		if ((c_pid = FORK_OR_VFORK()) == -1) {
> -			tst_brkm(TBROK, cleanup, "could not fork");
> -		}
> -
> -		if (c_pid == 0) {	/* child */
> -			/*
> -			 * Attempt to write another message to the full queue.
> -			 * Without the IPC_NOWAIT flag, the child sleeps
> -			 */
> -
> -#ifdef UCLINUX
> -			if (self_exec(av[0], "d", msg_q_1) < 0) {
> -				tst_brkm(TBROK, cleanup, "could not self_exec");
> -			}
> -#else
> -			do_child();
> -#endif
> -		} else {
> -			TST_PROCESS_STATE_WAIT(cleanup, c_pid, 'S');
> -
> -			/* send a signal that must be caught to the child */
> -			if (kill(c_pid, SIGHUP) == -1)
> -				tst_brkm(TBROK, cleanup, "kill failed");
> -
> -			waitpid(c_pid, NULL, 0);
> -		}
> -	}
> -
> -	cleanup();
> -
> -	tst_exit();
> -}
> -
> -/*
> - * do_child()
> - */
> -void do_child(void)
> -{
> -	TEST(msgsnd(msg_q_1, &msg_buf, MSGSIZE, 0));
> -
> +	TEST(msgsnd(queue_id, &snd_buf, MSGSIZE, tc->flag));
>  	if (TEST_RETURN != -1) {
> -		tst_resm(TFAIL, "call succeeded when error expected");
> -		exit(-1);
> +		tst_res(TFAIL, "msgsnd() succeeded unexpectedly");
> +		return;
>  	}
>  
> -	switch (TEST_ERRNO) {
> -	case EINTR:
> -		tst_resm(TPASS, "expected failure - errno = %d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> -		break;
> -	default:
> -		tst_resm(TFAIL,
> -			 "call failed with an unexpected error - %d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> -		break;
> +	if (TEST_ERRNO == tc->exp_err) {
> +		tst_res(TPASS | TTERRNO, "msgsnd() failed as expected");
> +	} else {
> +		tst_res(TFAIL | TTERRNO, "msgsnd() failed unexpectedly,"
> +			" expected %s", tst_strerrno(tc->exp_err));
>  	}
> -
> -	exit(0);
>  }
>  
> -void sighandler(int sig)
> +static void sighandler(int sig)
>  {
>  	if (sig == SIGHUP)
>  		return;
>  	else
> -		tst_brkm(TBROK, NULL, "received unexpected signal %d", sig);
> +		tst_brk(TBROK, "received unexpected signal %d", sig);

Using tst_brk() in signal handler is NOT safe.

Looking at the code the best solution would be to just call
_exit(TBROK); here instead, which is one of the things you can do safely
in a signal handler.

Otherwise it looks fine.

> diff --git a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd06.c b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd06.c
> index 42f6e79..737fc7f 100644
> --- a/testcases/kernel/syscalls/ipc/msgsnd/msgsnd06.c
> +++ b/testcases/kernel/syscalls/ipc/msgsnd/msgsnd06.c
> @@ -1,222 +1,92 @@
>  /*
> + * Copyright (c) International Business Machines  Corp., 2001
>   *
> - *   Copyright (c) International Business Machines  Corp., 2001
> + * 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 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.
>   *
> - *   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
> + * You should have received a copy of the GNU General Public License
> + * along with this program.
>   */
>  
>  /*
> - * NAME
> - *	msgsnd06.c
> - *
>   * DESCRIPTION
> - *	msgsnd06 - test for EIDRM error
> - *
> - * ALGORITHM
> - *	loop if that option was specified
> - *	create a message queue with read/write permissions
> - *	initialize a message buffer with a known message and type
> - *	enqueue messages in a loop until the queue is full
> - *	fork a child process
> - *	child attempts to enqueue a message to the full queue and sleeps
> - *	parent removes the queue and then exits
> - *	child get a return from msgsnd()
> - *	check the errno value
> - *	  issue a PASS message if we get EIDRM
> - *	otherwise, the tests fails
> - *	  issue a FAIL message
> - *	call cleanup
> - *
> - * USAGE:  <for command-line>
> - *  msgsnd06 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
> - *     where,  -c n : Run n copies concurrently.
> - *             -e   : Turn on errno logging.
> - *	       -i n : Execute test n times.
> - *	       -I x : Execute test for x seconds.
> - *	       -P x : Pause for x seconds between iterations.
> - *	       -t   : Turn on syscall timing.
> - *
> - * HISTORY
> - *	03/2001 - Written by Wayne Boyer
> - *      14/03/2008 Matthieu Fertr?? (Matthieu.Fertre@irisa.fr)
> - *      - Fix concurrency issue. Due to the use of usleep function to
> - *        synchronize processes, synchronization issues can occur on a loaded
> - *        system. Fix this by using pipes to synchronize processes.
> - *
> - * RESTRICTIONS
> - *	none
> + * test for EIDRM error

This may be a bit more detailed, i.e. something as:

Tests if EIDRM is returned when message queue was removed while msgsnd()
was trying to send a message.

> -#include <sys/wait.h>
> -#include "test.h"
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/ipc.h>
> +#include <sys/msg.h>
>  
> -#include "ipcmsg.h"
> +#include "tst_test.h"
> +#include "libnewipc.h"
>  
> -void cleanup(void);
> -void setup(void);
> -void do_child(void);
> +static key_t msgkey;
> +static int queue_id = -1;
> +static struct buf {
> +	long type;
> +	char text[MSGSIZE];
> +} snd_buf = {1, "hello, world"};
>  
> -char *TCID = "msgsnd06";
> -int TST_TOTAL = 1;
> -
> -int msg_q_1 = -1;		/* The message queue id created in setup */
> -
> -MSGBUF msg_buf;
> -
> -int main(int ac, char **av)
> +static void verify_msgsnd(void)
>  {
> -	int lc;
> -	pid_t c_pid;
> -	int status, e_code;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -#ifdef UCLINUX
> -#define PIPE_NAME	"msgsnd06"
> -	maybe_run_child(&do_child, "d", &msg_q_1);
> -#endif
> -
> -	setup();		/* global setup */
> -
> -	/* The following loop checks looping state if -i option given */
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		/* reset tst_count in case we are looping */
> -		tst_count = 0;
> -
> -		msgkey = getipckey();
> -
> -		/* create a message queue with read/write permission */
> -		if ((msg_q_1 = msgget(msgkey, IPC_CREAT | IPC_EXCL | MSG_RW))
> -		    == -1) {
> -			tst_brkm(TBROK, cleanup, "Can't create message queue");
> -		}
> -
> -		/* initialize the message buffer */
> -		init_buf(&msg_buf, MSGTYPE, MSGSIZE);
> -
> -		/* write messages to the queue until it is full */
> -		while (msgsnd(msg_q_1, &msg_buf, MSGSIZE, IPC_NOWAIT) != -1) {
> -			msg_buf.mtype += 1;
> -		}
> -
> -		/*
> -		 * fork a child that will attempt to write a message
> -		 * to the queue without IPC_NOWAIT
> -		 */
> -		if ((c_pid = FORK_OR_VFORK()) == -1) {
> -			tst_brkm(TBROK, cleanup, "could not fork");
> -		}
> -
> -		if (c_pid == 0) {	/* child */
> -
> -#ifdef UCLINUX
> -			if (self_exec(av[0], "d", msg_q_1) < 0) {
> -				tst_brkm(TBROK, cleanup, "could not self_exec");
> -			}
> -#else
> -			do_child();
> -#endif
> -		} else {
> -			TST_PROCESS_STATE_WAIT(cleanup, c_pid, 'S');
> -
> -			/* remove the queue */
> -			rm_queue(msg_q_1);
> -
> -			/* wait for the child to finish */
> -			wait(&status);
> -			/* make sure the child returned a good exit status */
> -			e_code = status >> 8;
> -			if (e_code != 0) {
> -				tst_resm(TFAIL, "Failures reported above");
> -			}
> -		}
> +	TEST(msgsnd(queue_id, &snd_buf, MSGSIZE, 0));
> +	if (TEST_RETURN != -1) {
> +		tst_res(TFAIL, "msgsnd() succeeded unexpectedly");
> +		return;
>  	}
>  
> -	cleanup();
> -
> -	tst_exit();
> +	if (TEST_ERRNO == EIDRM) {
> +		tst_res(TPASS | TTERRNO, "msgsnd() failed as expected");
> +	} else {
> +		tst_res(TFAIL | TTERRNO,
> +			"msgsnd() failed unexpectedly, expected EIDRM");
> +	}
>  }
>  
> -/*
> - * do_child()
> - */
> -void do_child(void)
> +static void do_test(void)
>  {
> -	int retval = 0;
> +	pid_t pid;
>  
> -#ifdef UCLINUX
> -	/* initialize the message buffer */
> -	init_buf(&msg_buf, MSGTYPE, MSGSIZE);
> +	queue_id = CREATE_MSG(msgkey, IPC_CREAT | IPC_EXCL | MSG_RW);
>  
> -#endif
> -	/*
> -	 * Attempt to write another message to the full queue.
> -	 * Without the IPC_NOWAIT flag, the child sleeps
> -	 */
> -	TEST(msgsnd(msg_q_1, &msg_buf, MSGSIZE, 0));
> +	while (msgsnd(queue_id, &snd_buf, MSGSIZE, IPC_NOWAIT) != -1)
> +		snd_buf.type += 1;
>  
> -	if (TEST_RETURN != -1) {
> -		retval = 1;
> -		tst_resm(TFAIL, "call succeeded when error expected");
> -		exit(retval);
> +	pid = SAFE_FORK();
> +	if (!pid) {
> +		verify_msgsnd();
> +		exit(0);
>  	}
>  
> -	switch (TEST_ERRNO) {
> -	case EIDRM:
> -		tst_resm(TPASS, "expected failure - errno = %d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> +	TST_PROCESS_STATE_WAIT(pid, 'S');
>  
> -		/* mark the queue as invalid as it was removed */
> -		msg_q_1 = -1;
> -		break;
> -	default:
> -		retval = 1;
> -		tst_resm(TFAIL,
> -			 "call failed with an unexpected error - %d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> -		break;
> +	if (queue_id != -1 && msgctl(queue_id, IPC_RMID, NULL)) {
> +		tst_res(TWARN | TERRNO, "failed to delete message queue %i",
> +				queue_id);

Uh, the queue_id canot be -1 here, right?

Also this is tst_brk(TBROK | TERRNO, ...) not TWARN, we are not in
cleanup here.

And the queue_id should be reset to -1 right after it was removed
succesfully and there should be a cleanup that removes the queue if
queue_id is not set to -1, because the test may still call tst_brk()
somewhere between the creation and removal of the queue. We have to be
careful with System V queues since these are global persistent objects
that can outlive the test easily.

>  	}
> -	exit(retval);
> -}
> -
> -/*
> - * setup() - performs all the ONE TIME setup for this test.
> - */
> -void setup(void)
> -{
> -
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
>  
> -	/*
> -	 * Create a temporary directory and cd into it.
> -	 * This helps to ensure that a unique msgkey is created.
> -	 * See ../lib/libipc.c for more information.
> -	 */
> -	tst_tmpdir();
> +	tst_reap_children();
>  }
>  
> -/*
> - * cleanup() - performs all the ONE TIME cleanup for this test at completion
> - * 	       or premature exit.
> - */
> -void cleanup(void)
> +static void setup(void)
>  {
> -
> -	tst_rmdir();
> -
> +	msgkey = GETIPCKEY();
>  }
> +
> +static struct tst_test test = {
> +	.tid = "msgsnd06",
> +	.needs_tmpdir = 1,
> +	.needs_root = 1,
> +	.forks_child = 1,
> +	.setup = setup,
> +	.test_all = do_test
> +};
> -- 
> 1.8.3.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz