Re: [LTP] [PATCH v7 1/5] Refactor ioctl02.c to use the new test API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +	parentpid = getpid();
> +	childpid = SAFE_FORK();
> +	if (!childpid) {
> +		childfd = do_child_setup();
> +		if (childfd < 0)
> +			_exit(1);
> +		run_ctest();
> +		_exit(0);

This is stil mostly useless code, the do_child_setup() now uses safe
macros and as such cannot fail, the whole test will be aborted if any of
the SAFE_*() calls fails.

Hence there is no point in attempting to exit(1) because that is never
reached. The split of the child into two function does not make any
sense anymore. So this should really be:

	if (!childpid) {
		do_child();
		exit(0);
	}

>  	}
> -	cleanup();
>  
> -	tst_exit();
> -}
> +	TST_CHECKPOINT_WAIT(0);
>  
> -static void do_child(void)
> -{
> -	childfd = do_child_setup();
> -	if (childfd < 0)
> -		_exit(1);
> -	run_ctest();
> -	_exit(0);
> -}
> -
> -void do_child_uclinux(void)
> -{
> -	struct sigaction act;
> +	parentfd = SAFE_OPEN(parenttty, O_RDWR, 0777);
> +	/* flush tty queues to remove old output */
> +	SAFE_IOCTL(parentfd, TCFLSH, 2);
>  
> -	/* Set up the signal handlers again */
> -	act.sa_handler = (void *)sigterm_handler;
> -	act.sa_flags = 0;
> -	sigemptyset(&act.sa_mask);
> -	(void)sigaction(SIGTERM, &act, 0);
> +	/* run the parent test */
> +	run_ptest();
>  
> -	/* Run the normal child */
> -	do_child();
> +	TST_CHECKPOINT_WAKE(0);
>  }
>  
>  /*
>   * run_ptest() - setup the various termio structure values and issue
>   *		 the TCSETA ioctl call with the TEST macro.
>   */
> -static int run_ptest(void)
> +static void run_ptest(void)
>  {
> -	int i, rval;
> -
>  	/* Use "old" line discipline */
>  	termio.c_line = 0;
>  
> @@ -230,7 +93,7 @@ static int run_ptest(void)
>  	termio.c_cflag = B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL;
>  
>  	/* Set control chars. */
> -	for (i = 0; i < NCC; i++) {
> +	for (int i = 0; i < NCC; i++) {
>  		if (i == VEOL2)
>  			continue;
>  		termio.c_cc[i] = CSTART;
> @@ -248,47 +111,26 @@ static int run_ptest(void)
>  	/* Set output modes. */
>  	termio.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
>  
> -	TEST(ioctl(parentfd, TCSETA, &termio));
> -
> -	if (TEST_RETURN < 0) {
> -		tst_resm(TFAIL, "ioctl TCSETA failed : "
> -			 "errno = %d", TEST_ERRNO);
> -		return -1;
> -	}
> +	SAFE_IOCTL(parentfd, TCSETA, &termio);
>  
>  	/* Get termio and see if all parameters actually got set */
> -	rval = ioctl(parentfd, TCGETA, &termio);
> -	if (rval < 0) {
> -		tst_resm(TFAIL, "ioctl TCGETA failed.  Ending test.");
> -		return -1;
> -	}
> -
> -	return chk_tty_parms();
> +	SAFE_IOCTL(parentfd, TCGETA, &termio);
> +	chk_tty_parms();
>  }
>  
> -static int run_ctest(void)
> +static void run_ctest(void)
>  {
> -	/*
> -	 * Wait till the parent has finished testing.
> -	 */
> -	while (!sigterm)
> -		sleep(1);
> -
> -	sigterm = 0;
> -
> -	tst_resm(TINFO, "child: Got SIGTERM from parent.");
> -
> -	if (close(childfd) == -1)
> -		tst_resm(TINFO, "close() in run_ctest() failed");
> -	return 0;
> +	TST_CHECKPOINT_WAIT(0);
> +	tst_res(TINFO, "child: parent has finished testing");
> +	SAFE_CLOSE(childfd);
>  }
>  
> -static int chk_tty_parms(void)
> +static void chk_tty_parms(void)
>  {
>  	int i, flag = 0;
>  
>  	if (termio.c_line != 0) {
> -		tst_resm(TINFO, "line discipline has incorrect value %o",
> +		tst_res(TINFO, "line discipline has incorrect value %o",
>  			 termio.c_line);
>  		flag++;
>  	}
> @@ -301,7 +143,7 @@ static int chk_tty_parms(void)
>  	 */
>  #if 0
>  	if (termio.c_cflag != (B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL)) {
> -		tst_resm(TINFO, "cflag has incorrect value. %o",
> +		tst_res(TINFO, "cflag has incorrect value. %o",
>  			 termio.c_cflag);
>  		flag++;
>  	}
> @@ -309,19 +151,18 @@ static int chk_tty_parms(void)
>  
>  	for (i = 0; i < NCC; i++) {
>  		if (i == VEOL2) {
> -			if (termio.c_cc[VEOL2] == CNUL) {
> +			if (!termio.c_cc[i]) {
>  				continue;
>  			} else {
> -				tst_resm(TINFO, "control char %d has "
> -					 "incorrect value %d %d", i,
> -					 termio.c_cc[i], CNUL);
> +				tst_res(TINFO, "control char %d has "
> +					 "incorrect value %d", i, termio.c_cc[i]);
>  				flag++;
>  				continue;
>  			}
>  		}
>  
>  		if (termio.c_cc[i] != CSTART) {
> -			tst_resm(TINFO, "control char %d has incorrect "
> +			tst_res(TINFO, "control char %d has incorrect "
>  				 "value %d.", i, termio.c_cc[i]);
>  			flag++;
>  		}
> @@ -330,7 +171,7 @@ static int chk_tty_parms(void)
>  	if (!
>  	    (termio.c_lflag
>  	     && (ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH))) {
> -		tst_resm(TINFO, "lflag has incorrect value. %o",
> +		tst_res(TINFO, "lflag has incorrect value. %o",
>  			 termio.c_lflag);
>  		flag++;
>  	}
> @@ -339,130 +180,68 @@ static int chk_tty_parms(void)
>  	    (termio.c_iflag
>  	     && (BRKINT | IGNPAR | INPCK | ISTRIP | ICRNL | IUCLC | IXON | IXANY
>  		 | IXOFF))) {
> -		tst_resm(TINFO, "iflag has incorrect value. %o",
> +		tst_res(TINFO, "iflag has incorrect value. %o",
>  			 termio.c_iflag);
>  		flag++;
>  	}
>  
>  	if (!(termio.c_oflag && (OPOST | OLCUC | ONLCR | ONOCR))) {
> -		tst_resm(TINFO, "oflag has incorrect value. %o",
> +		tst_res(TINFO, "oflag has incorrect value. %o",
>  			 termio.c_oflag);
>  		flag++;
>  	}
>  
> -	if (!flag)
> -		tst_resm(TINFO, "termio values are set as expected");
> -
> -	return flag;
> -}
> -
> -static int do_parent_setup(void)
> -{
> -	int pfd;
> -
> -	pfd = SAFE_OPEN(cleanup, parenttty, O_RDWR, 0777);
> -
> -	/* unset the closed flag */
> -	closed = 0;
> -
> -	/* flush tty queues to remove old output */
> -	SAFE_IOCTL(cleanup, pfd, TCFLSH, 2);
> -	return pfd;
> +	if (flag != 0)
> +		tst_res(TFAIL, "TCGETA/TCSETA tests FAILED with "
> +				"%d %s", flag, flag > 1 ? "errors" : "error");
> +	else
> +		tst_res(TPASS, "TCGETA/TCSETA tests SUCCEEDED");

I would prefer changing the TINFO statements above to TFAIL and simply
add:

	if (!flag)
		tst_res(TPASS, "TCGETA/TCSETA ioctl() flags are correct"


Since that actually makes it clear which part of the test failed.

Also you shouldn't add SUCCEEDED into the messages, the fact that the
test passed is printed by the test library already because of the TPASS
already.

>  }
>  
>  static int do_child_setup(void)
>  {
> -	int cfd;
> -
> -	cfd = open(childtty, O_RDWR, 0777);
> -	if (cfd < 0) {
> -		tst_resm(TINFO, "Could not open %s in do_child_setup(), errno "
> -			 "= %d", childtty, errno);
> -		/* signal the parent so we don't hang the test */
> -		kill(parentpid, SIGUSR1);
> -		return -1;
> -	}
> +	int cfd = SAFE_OPEN(childtty, O_RDWR, 0777);
>  
>  	/* flush tty queues to remove old output */
> -	if (ioctl(cfd, TCFLSH, 2) < 0) {
> -		tst_resm(TINFO, "ioctl TCFLSH failed. : errno = %d", errno);
> -		/* signal the parent so we don't hang the test */
> -		kill(parentpid, SIGUSR1);
> -		return -1;
> -	}
> +	SAFE_IOCTL(cfd, TCFLSH, 2);
>  
>  	/* tell the parent that we're done */
> -	kill(parentpid, SIGUSR1);
> -
> +	TST_CHECKPOINT_WAKE(0);
>  	return cfd;
>  }
>  
> -/*
> - * Define the signals handlers here.
> - */
> -static void sigterm_handler(void)
> -{
> -	sigterm = 1;
> -}
> -
> -static void sigusr1_handler(void)
> -{
> -	sigusr1 = 1;
> -}
> -
> -static void sigusr2_handler(void)
> -{
> -	sigusr2 = 1;
> -}
> -
> -static void help(void)
> -{
> -	printf("  -D <tty device> : for example, /dev/tty[0-9]\n");
> -}
> -
>  static void setup(void)
>  {
> -	int fd;
> -	struct sigaction act;
> +	if (!device)
> +		tst_brk(TBROK, "You must specify a tty device with -D option");
>  
>  	/* XXX: TERRNO required all over the place */

This comment is no longer true.

> -	fd = SAFE_OPEN(NULL, devname, O_RDWR, 0777);
> +	int fd = SAFE_OPEN(device, O_RDWR, 0777);
>  
>  	/* Save the current device information - to be restored in cleanup() */
> -	SAFE_IOCTL(cleanup, fd, TCGETA, &save_io);
> +	SAFE_IOCTL(fd, TCGETA, &save_io);
>  
>  	/* Close the device */

And this comment is obvious. Generally the test is overcommented, I
would just remove most of the comments, since they are either obvious or
no longer true after the changes.

> -	SAFE_CLOSE(cleanup, fd);

-- 
Cyril Hrubis
chrubis@suse.cz

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