[LTP] [PATCH v7 0/5] Improve ioctl02.c

[LTP] [PATCH v7 0/5] Improve ioctl02.c

[Marius Kittler]

I implemented the changes that were requested in the last review.
So I amended patch 1 and added patch 5.

I'm not sure how to write generic code in C (in C++ I would just
use templates). So I duplicated the functions that access the
members of the structs termio and termios to test both. Turning
these functions into huge multi-line macros would likely be the
worse choice.

Marius Kittler (5):
  Refactor ioctl02.c to use the new test API
  Make checks for termio flags more strict
  Remove disabled code in ioctl02.c
  Use termios instead of legacy struct in ioctl02.c
  Extend ioctl02 to test termio and termios

 testcases/kernel/syscalls/ioctl/ioctl02.c | 542 +++++++++-------------
 1 file changed, 214 insertions(+), 328 deletions(-)

-- 
2.42.0


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

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

[Marius Kittler]

* Use checkpoint API instead of signals
* Use SAFE_…-macros instead of manual error handling
* See https://github.com/linux-test-project/ltp/issues/637
  for related discussion.

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 testcases/kernel/syscalls/ioctl/ioctl02.c | 411 +++++-----------------
 1 file changed, 95 insertions(+), 316 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index b4d4a3594..b0376748d 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -1,228 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *   Copyright (c) International Business Machines  Corp., 2001
- *   Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
- *
- *   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.
- *
- *   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
+ * Copyright (c) International Business Machines  Corp., 2001
+ * Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
+ * Copyright (c) 2023 Marius Kittler <mkittler@suse.de>
  */
 
-/*
- * NAME
- *	ioctl02.c
- *
- * DESCRIPTION
- *	Testcase to test the TCGETA, and TCSETA ioctl implementations for
- *	the tty driver
- *
- * ALGORITHM
- *	In this test, the parent and child open the parentty and the childtty
- *	respectively.  After opening the childtty the child flushes the stream
- *	and sends a SIGUSR1 to the parent (thereby asking it to continue its
- *	testing). The parent, which was waiting for this signal to arrive, now
- *	starts the testing. It issues a TCGETA ioctl to get all the tty
- *	parameters. It then changes them to known values by issuing a TCSETA
- *	ioctl.  Then the parent issues a TCGETA ioctl again and compares the
- *	received values with what it had set earlier. The test fails if TCGETA
- *	or TCSETA fails, or if the received values don't match those that were
- *	set. The parent does all the testing, the requirement of the child
- *	process is to moniter the testing done by the parent, and hence the
- *	child just waits for the parent.
+/*\
+ * [Description]
  *
- * USAGE:  <for command-line>
- *  ioctl02 -D /dev/tty[0-9] [-c n] [-f] [-i n] [-I x] [-P x] [-t]
- *     where,  -c n : Run n copies concurrently.
- *             -f   : Turn off functionality Testing.
- *             -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.
+ * Testcase to test the TCGETA, and TCSETA ioctl implementations for
+ * the tty driver
  *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *
- * RESTRICTIONS
- *	test must be run with the -D option
- *	test may have to be run as root depending on the tty permissions
+ * In this test, the parent and child open the parentty and the childtty
+ * respectively.  After opening the childtty the child flushes the stream
+ * and wakes the parent (thereby asking it to continue its testing). The
+ * parent, then starts the testing. It issues a TCGETA ioctl to get all
+ * the tty parameters. It then changes them to known values by issuing a
+ * TCSETA ioctl. Then the parent issues a TCGETA ioctl again and compares
+ * the received values with what it had set earlier. The test fails if
+ * TCGETA or TCSETA fails, or if the received values don't match those
+ * that were set. The parent does all the testing, the requirement of the
+ * child process is to moniter the testing done by the parent, and hence
+ * the child just waits for the parent.
  */
 
 #include <stdio.h>
 #include <fcntl.h>
-#include <signal.h>
 #include <errno.h>
 #include <sys/wait.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <termios.h>
-#include "test.h"
-#include "safe_macros.h"
-#include "lapi/ioctl.h"
 
-#define	CNUL	0
+#include "lapi/ioctl.h"
 
-char *TCID = "ioctl02";
-int TST_TOTAL = 1;
+#include "tst_checkpoint.h"
+#include "tst_test.h"
+#include "tst_safe_macros.h"
 
 static struct termio termio, save_io;
 
 static char *parenttty, *childtty;
-static int parentfd, childfd;
+static int parentfd = -1, childfd = -1;
 static int parentpid, childpid;
-static volatile int sigterm, sigusr1, sigusr2;
-static int closed = 1;
 
 static int do_child_setup(void);
-static int do_parent_setup(void);
-static int run_ptest(void);
-static int run_ctest(void);
-static int chk_tty_parms();
+static void run_ptest(void);
+static void run_ctest(void);
+static void chk_tty_parms(void);
 static void setup(void);
 static void cleanup(void);
-static void help(void);
-static void do_child(void);
-void do_child_uclinux(void);
-static void sigterm_handler(void);
 
-static int Devflag;
-static char *devname;
+static char *device;
 
-static option_t options[] = {
-	{"D:", &Devflag, &devname},
-	{NULL, NULL, NULL}
-};
-
-int main(int ac, char **av)
+static void verify_ioctl(void)
 {
-	int lc;
-	int rval;
-
-	tst_parse_opts(ac, av, options, &help);
-
-#ifdef UCLINUX
-	maybe_run_child(&do_child_uclinux, "dS", &parentpid, &childtty);
-#endif
-
-	if (!Devflag)
-		tst_brkm(TBROK, NULL, "You must specify a tty device with "
-			 "the -D option.");
-
-	tst_require_root();
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		parenttty = devname;
-		childtty = devname;
-
-		parentpid = getpid();
-
-		childpid = FORK_OR_VFORK();
-		if (childpid < 0)
-			tst_brkm(TBROK, cleanup, "fork failed");
-
-		if (childpid == 0) {	/* child */
-#ifdef UCLINUX
-			if (self_exec(av[0], "dS", parentpid, childtty) < 0)
-				tst_brkm(TBROK, cleanup, "self_exec failed");
-#else
-			do_child();
-#endif
-		}
-
-		while (!sigusr1)
-			sleep(1);
-
-		sigusr1 = 0;
-
-		parentfd = do_parent_setup();
-		if (parentfd < 0) {
-			kill(childpid, SIGTERM);
-			waitpid(childpid, NULL, 0);
-			cleanup();
-		}
-
-		/* run the parent test */
-		rval = run_ptest();
-		if (rval == -1) {
-			/*
-			 * Parent cannot set/get ioctl parameters.
-			 * SIGTERM the child and cleanup.
-			 */
-			kill(childpid, SIGTERM);
-			waitpid(childpid, NULL, 0);
-			cleanup();
-		}
-
-		if (rval != 0)
-			tst_resm(TFAIL, "TCGETA/TCSETA tests FAILED with "
-				 "%d %s", rval, rval > 1 ? "errors" : "error");
-		else
-			tst_resm(TPASS, "TCGETA/TCSETA tests SUCCEEDED");
-
-		/* FIXME: check return codes. */
-		(void)kill(childpid, SIGTERM);
-		(void)waitpid(childpid, NULL, 0);
-
-		/*
-		 * Clean up things from the parent by restoring the
-		 * tty device information that was saved in setup()
-		 * and closing the tty file descriptor.
-		 */
-		if (ioctl(parentfd, TCSETA, &save_io) == -1)
-			tst_resm(TINFO, "ioctl restore failed in main");
-		SAFE_CLOSE(cleanup, parentfd);
-
-		closed = 1;
+	parenttty = device;
+	childtty = device;
+
+	parentpid = getpid();
+	childpid = SAFE_FORK();
+	if (!childpid) {
+		childfd = do_child_setup();
+		if (childfd < 0)
+			_exit(1);
+		run_ctest();
+		_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");
 }
 
 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 */
-	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 */
-	SAFE_CLOSE(cleanup, fd);
-
-	/* Set up the signal handlers */
-	act.sa_handler = (void *)sigterm_handler;
-	act.sa_flags = 0;
-	sigemptyset(&act.sa_mask);
-	(void)sigaction(SIGTERM, &act, 0);
-
-	act.sa_handler = (void *)sigusr1_handler;
-	act.sa_flags = 0;
-	(void)sigaction(SIGUSR1, &act, 0);
-
-	act.sa_handler = (void *)sigusr2_handler;
-	act.sa_flags = 0;
-	(void)sigaction(SIGUSR2, &act, 0);
-
-	act.sa_handler = SIG_IGN;
-	act.sa_flags = 0;
-	(void)sigaction(SIGTTOU, &act, 0);
-
-	sigterm = sigusr1 = sigusr2 = 0;
-
-	TEST_PAUSE;
+	SAFE_CLOSE(fd);
 }
 
 static void cleanup(void)
 {
-	if (!closed) {
-		if (ioctl(parentfd, TCSETA, &save_io) == -1)
-			tst_resm(TINFO, "ioctl restore failed in cleanup()");
-		if (close(parentfd) == -1)
-			tst_resm(TINFO, "close() failed in cleanup()");
+	if (parentfd >= 0) {
+		SAFE_IOCTL(parentfd, TCSETA, &save_io);
+		SAFE_CLOSE(parentfd);
 	}
 }
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_checkpoints = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = verify_ioctl,
+	.options = (struct tst_option[]) {
+		{"D:", &device, "Tty device. For example, /dev/tty[0-9]"},
+		{}
+	}
+};
\ No newline at end of file
-- 
2.42.0


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

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

[Cyril Hrubis]

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

[LTP] [PATCH v7 2/5] Make checks for termio flags more strict

[Marius Kittler]

The checks for termio flags can actually use a `!=` check to
also fail when any unexpected flags are present.

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 testcases/kernel/syscalls/ioctl/ioctl02.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index b0376748d..25b83d358 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -168,24 +168,21 @@ static void chk_tty_parms(void)
 		}
 	}
 
-	if (!
-	    (termio.c_lflag
-	     && (ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH))) {
+	if (termio.c_lflag != (ISIG | ICANON | XCASE | ECHO | ECHOE
+		 | NOFLSH)) {
 		tst_res(TINFO, "lflag has incorrect value. %o",
 			 termio.c_lflag);
 		flag++;
 	}
 
-	if (!
-	    (termio.c_iflag
-	     && (BRKINT | IGNPAR | INPCK | ISTRIP | ICRNL | IUCLC | IXON | IXANY
-		 | IXOFF))) {
+	if (termio.c_iflag != (BRKINT | IGNPAR | INPCK | ISTRIP
+		 | ICRNL | IUCLC | IXON | IXANY | IXOFF)) {
 		tst_res(TINFO, "iflag has incorrect value. %o",
 			 termio.c_iflag);
 		flag++;
 	}
 
-	if (!(termio.c_oflag && (OPOST | OLCUC | ONLCR | ONOCR))) {
+	if (termio.c_oflag != (OPOST | OLCUC | ONLCR | ONOCR)) {
 		tst_res(TINFO, "oflag has incorrect value. %o",
 			 termio.c_oflag);
 		flag++;
-- 
2.42.0


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

[LTP] [PATCH v7 3/5] Remove disabled code in ioctl02.c

[Marius Kittler]

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 testcases/kernel/syscalls/ioctl/ioctl02.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index 25b83d358..c66473a8d 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -134,20 +134,6 @@ static void chk_tty_parms(void)
 			 termio.c_line);
 		flag++;
 	}
-	/*
-	 * The following Code Sniffet is disabled to check the value of c_cflag
-	 * as it seems that due to some changes from 2.6.24 onwards, this
-	 * setting is not done properly for either of (B50|CS7|CREAD|PARENB|
-	 * PARODD|CLOCAL|(CREAD|HUPCL|CLOCAL).
-	 * However, it has been observed that other flags are properly set.
-	 */
-#if 0
-	if (termio.c_cflag != (B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL)) {
-		tst_res(TINFO, "cflag has incorrect value. %o",
-			 termio.c_cflag);
-		flag++;
-	}
-#endif
 
 	for (i = 0; i < NCC; i++) {
 		if (i == VEOL2) {
-- 
2.42.0


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

Re: [LTP] [PATCH v7 3/5] Remove disabled code in ioctl02.c

[Martin Doucha]

Hi,
the first 3 patches can be merged as is. For all patches up to here:

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 23. 10. 23 18:02, Marius Kittler wrote:
> Signed-off-by: Marius Kittler <mkittler@suse.de>
> ---
>   testcases/kernel/syscalls/ioctl/ioctl02.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
> index 25b83d358..c66473a8d 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl02.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
> @@ -134,20 +134,6 @@ static void chk_tty_parms(void)
>   			 termio.c_line);
>   		flag++;
>   	}
> -	/*
> -	 * The following Code Sniffet is disabled to check the value of c_cflag
> -	 * as it seems that due to some changes from 2.6.24 onwards, this
> -	 * setting is not done properly for either of (B50|CS7|CREAD|PARENB|
> -	 * PARODD|CLOCAL|(CREAD|HUPCL|CLOCAL).
> -	 * However, it has been observed that other flags are properly set.
> -	 */
> -#if 0
> -	if (termio.c_cflag != (B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL)) {
> -		tst_res(TINFO, "cflag has incorrect value. %o",
> -			 termio.c_cflag);
> -		flag++;
> -	}
> -#endif
>   
>   	for (i = 0; i < NCC; i++) {
>   		if (i == VEOL2) {

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

[LTP] [PATCH v7 4/5] Use termios instead of legacy struct in ioctl02.c

[Marius Kittler]

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 testcases/kernel/syscalls/ioctl/ioctl02.c | 24 +++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index c66473a8d..5111d7178 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -8,17 +8,17 @@
 /*\
  * [Description]
  *
- * Testcase to test the TCGETA, and TCSETA ioctl implementations for
+ * Testcase to test the TCGETS, and TCSETS ioctl implementations for
  * the tty driver
  *
  * In this test, the parent and child open the parentty and the childtty
  * respectively.  After opening the childtty the child flushes the stream
  * and wakes the parent (thereby asking it to continue its testing). The
- * parent, then starts the testing. It issues a TCGETA ioctl to get all
+ * parent, then starts the testing. It issues a TCGETS ioctl to get all
  * the tty parameters. It then changes them to known values by issuing a
- * TCSETA ioctl. Then the parent issues a TCGETA ioctl again and compares
+ * TCSETS ioctl. Then the parent issues a TCGETS ioctl again and compares
  * the received values with what it had set earlier. The test fails if
- * TCGETA or TCSETA fails, or if the received values don't match those
+ * TCGETS or TCSETS fails, or if the received values don't match those
  * that were set. The parent does all the testing, the requirement of the
  * child process is to moniter the testing done by the parent, and hence
  * the child just waits for the parent.
@@ -38,7 +38,7 @@
 #include "tst_test.h"
 #include "tst_safe_macros.h"
 
-static struct termio termio, save_io;
+static struct termios termio, save_io;
 
 static char *parenttty, *childtty;
 static int parentfd = -1, childfd = -1;
@@ -82,7 +82,7 @@ static void verify_ioctl(void)
 
 /*
  * run_ptest() - setup the various termio structure values and issue
- *		 the TCSETA ioctl call with the TEST macro.
+ *		 the TCSETS ioctl call with the TEST macro.
  */
 static void run_ptest(void)
 {
@@ -111,10 +111,10 @@ static void run_ptest(void)
 	/* Set output modes. */
 	termio.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
 
-	SAFE_IOCTL(parentfd, TCSETA, &termio);
+	SAFE_IOCTL(parentfd, TCSETS, &termio);
 
 	/* Get termio and see if all parameters actually got set */
-	SAFE_IOCTL(parentfd, TCGETA, &termio);
+	SAFE_IOCTL(parentfd, TCGETS, &termio);
 	chk_tty_parms();
 }
 
@@ -175,10 +175,10 @@ static void chk_tty_parms(void)
 	}
 
 	if (flag != 0)
-		tst_res(TFAIL, "TCGETA/TCSETA tests FAILED with "
+		tst_res(TFAIL, "TCGETS/TCSETS tests FAILED with "
 				"%d %s", flag, flag > 1 ? "errors" : "error");
 	else
-		tst_res(TPASS, "TCGETA/TCSETA tests SUCCEEDED");
+		tst_res(TPASS, "TCGETS/TCSETS tests SUCCEEDED");
 }
 
 static int do_child_setup(void)
@@ -202,7 +202,7 @@ static void setup(void)
 	int fd = SAFE_OPEN(device, O_RDWR, 0777);
 
 	/* Save the current device information - to be restored in cleanup() */
-	SAFE_IOCTL(fd, TCGETA, &save_io);
+	SAFE_IOCTL(fd, TCGETS, &save_io);
 
 	/* Close the device */
 	SAFE_CLOSE(fd);
@@ -211,7 +211,7 @@ static void setup(void)
 static void cleanup(void)
 {
 	if (parentfd >= 0) {
-		SAFE_IOCTL(parentfd, TCSETA, &save_io);
+		SAFE_IOCTL(parentfd, TCSETS, &save_io);
 		SAFE_CLOSE(parentfd);
 	}
 }
-- 
2.42.0


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

[LTP] [PATCH v7 5/5] Extend ioctl02 to test termio and termios

[Marius Kittler]

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 testcases/kernel/syscalls/ioctl/ioctl02.c | 170 +++++++++++++++++++---
 1 file changed, 147 insertions(+), 23 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index 5111d7178..00d51b367 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -8,22 +8,24 @@
 /*\
  * [Description]
  *
- * Testcase to test the TCGETS, and TCSETS ioctl implementations for
- * the tty driver
+ * Testcase to test the TCGETA/TCGETS, and TCSETA/TCSETS ioctl
+ * implementations for the tty driver
  *
  * In this test, the parent and child open the parentty and the childtty
  * respectively.  After opening the childtty the child flushes the stream
  * and wakes the parent (thereby asking it to continue its testing). The
- * parent, then starts the testing. It issues a TCGETS ioctl to get all
- * the tty parameters. It then changes them to known values by issuing a
- * TCSETS ioctl. Then the parent issues a TCGETS ioctl again and compares
- * the received values with what it had set earlier. The test fails if
- * TCGETS or TCSETS fails, or if the received values don't match those
- * that were set. The parent does all the testing, the requirement of the
- * child process is to moniter the testing done by the parent, and hence
- * the child just waits for the parent.
+ * parent, then starts the testing. It issues a TCGETA/TCGETS ioctl to
+ * get all the tty parameters. It then changes them to known values by
+ * issuing a TCSETA/TCSETS ioctl. Then the parent issues a TCSETA/TCGETS
+ * ioctl again and compares the received values with what it had set
+ * earlier. The test fails if TCGETA/TCGETS or TCSETA/TCSETS fails, or if
+ * the received values don't match those that were set. The parent does
+ * all the testing, the requirement of the child process is to moniter
+ * the testing done by the parent, and hence the child just waits for the
+ * parent.
  */
 
+#include <asm-generic/ioctls.h>
 #include <stdio.h>
 #include <fcntl.h>
 #include <errno.h>
@@ -38,23 +40,54 @@
 #include "tst_test.h"
 #include "tst_safe_macros.h"
 
-static struct termios termio, save_io;
+static struct termio termio;
+static struct termios termios, termios_bak;
 
 static char *parenttty, *childtty;
 static int parentfd = -1, childfd = -1;
 static int parentpid, childpid;
 
 static int do_child_setup(void);
+static void prepare_termio(void);
+static void preapre_termios(void);
 static void run_ptest(void);
 static void run_ctest(void);
-static void chk_tty_parms(void);
+static void chk_tty_parms_termio(void);
+static void chk_tty_parms_termios(void);
 static void setup(void);
 static void cleanup(void);
 
 static char *device;
 
+static struct variant {
+	const char *name;
+	void *termio, *termio_bak;
+	void (*prepare)(void);
+	void (*check)(void);
+	int tcget, tcset;
+} variants[] = {
+	{
+		.name = "termio",
+		.termio = &termio,
+		.prepare = &prepare_termio,
+		.check = &chk_tty_parms_termio,
+		.tcget = TCGETA,
+		.tcset = TCSETA,
+	},
+	{
+		.name = "termios",
+		.termio = &termios,
+		.prepare = preapre_termios,
+		.check = &chk_tty_parms_termios,
+		.tcget = TCGETS,
+		.tcset = TCSETS,
+	},
+};
+
 static void verify_ioctl(void)
 {
+	tst_res(TINFO, "Testing %s variant", variants[tst_variant].name);
+
 	parenttty = device;
 	childtty = device;
 
@@ -80,11 +113,7 @@ static void verify_ioctl(void)
 	TST_CHECKPOINT_WAKE(0);
 }
 
-/*
- * run_ptest() - setup the various termio structure values and issue
- *		 the TCSETS ioctl call with the TEST macro.
- */
-static void run_ptest(void)
+static void prepare_termio(void)
 {
 	/* Use "old" line discipline */
 	termio.c_line = 0;
@@ -110,12 +139,50 @@ static void run_ptest(void)
 
 	/* Set output modes. */
 	termio.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
+}
 
-	SAFE_IOCTL(parentfd, TCSETS, &termio);
+static void preapre_termios(void)
+{
+	/* Use "old" line discipline */
+	termios.c_line = 0;
+
+	/* Set control modes */
+	termios.c_cflag = B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL;
+
+	/* Set control chars. */
+	for (int i = 0; i < NCC; i++) {
+		if (i == VEOL2)
+			continue;
+		termios.c_cc[i] = CSTART;
+	}
+
+	/* Set local modes. */
+	termios.c_lflag =
+	    ((unsigned short)(ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH));
+
+	/* Set input modes. */
+	termios.c_iflag =
+	    BRKINT | IGNPAR | INPCK | ISTRIP | ICRNL | IUCLC | IXON | IXANY |
+	    IXOFF;
+
+	/* Set output modes. */
+	termios.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
+}
+
+/*
+ * run_ptest() - setup the various termio/termios structure values and issue
+ * the TCSETA/TCSETS ioctl call with the TEST macro.
+ */
+static void run_ptest(void)
+{
+	struct variant *v = &variants[tst_variant];
+
+	v->prepare();
+	SAFE_IOCTL(parentfd, v->tcset, v->termio);
 
 	/* Get termio and see if all parameters actually got set */
-	SAFE_IOCTL(parentfd, TCGETS, &termio);
-	chk_tty_parms();
+	SAFE_IOCTL(parentfd, v->tcget, v->termio);
+	v->check();
 }
 
 static void run_ctest(void)
@@ -125,7 +192,7 @@ static void run_ctest(void)
 	SAFE_CLOSE(childfd);
 }
 
-static void chk_tty_parms(void)
+static void chk_tty_parms_termio(void)
 {
 	int i, flag = 0;
 
@@ -174,6 +241,62 @@ static void chk_tty_parms(void)
 		flag++;
 	}
 
+	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");
+}
+
+static void chk_tty_parms_termios(void)
+{
+	int i, flag = 0;
+
+	if (termios.c_line != 0) {
+		tst_res(TINFO, "line discipline has incorrect value %o",
+			 termios.c_line);
+		flag++;
+	}
+
+	for (i = 0; i < NCC; i++) {
+		if (i == VEOL2) {
+			if (!termios.c_cc[i]) {
+				continue;
+			} else {
+				tst_res(TINFO, "control char %d has "
+					 "incorrect value %d", i, termios.c_cc[i]);
+				flag++;
+				continue;
+			}
+		}
+
+		if (termios.c_cc[i] != CSTART) {
+			tst_res(TINFO, "control char %d has incorrect "
+				 "value %d.", i, termios.c_cc[i]);
+			flag++;
+		}
+	}
+
+	if (termios.c_lflag != (ISIG | ICANON | XCASE | ECHO | ECHOE
+		 | NOFLSH)) {
+		tst_res(TINFO, "lflag has incorrect value. %o",
+			 termios.c_lflag);
+		flag++;
+	}
+
+	if (termios.c_iflag != (BRKINT | IGNPAR | INPCK | ISTRIP
+		 | ICRNL | IUCLC | IXON | IXANY | IXOFF)) {
+		tst_res(TINFO, "iflag has incorrect value. %o",
+			 termios.c_iflag);
+		flag++;
+	}
+
+	if (termios.c_oflag != (OPOST | OLCUC | ONLCR | ONOCR)) {
+		tst_res(TINFO, "oflag has incorrect value. %o",
+			 termios.c_oflag);
+		flag++;
+	}
+
 	if (flag != 0)
 		tst_res(TFAIL, "TCGETS/TCSETS tests FAILED with "
 				"%d %s", flag, flag > 1 ? "errors" : "error");
@@ -202,7 +325,7 @@ static void setup(void)
 	int fd = SAFE_OPEN(device, O_RDWR, 0777);
 
 	/* Save the current device information - to be restored in cleanup() */
-	SAFE_IOCTL(fd, TCGETS, &save_io);
+	SAFE_IOCTL(fd, TCGETS, &termios_bak);
 
 	/* Close the device */
 	SAFE_CLOSE(fd);
@@ -211,7 +334,7 @@ static void setup(void)
 static void cleanup(void)
 {
 	if (parentfd >= 0) {
-		SAFE_IOCTL(parentfd, TCSETS, &save_io);
+		SAFE_IOCTL(parentfd, TCSETS, &termios_bak);
 		SAFE_CLOSE(parentfd);
 	}
 }
@@ -223,6 +346,7 @@ static struct tst_test test = {
 	.setup = setup,
 	.cleanup = cleanup,
 	.test_all = verify_ioctl,
+	.test_variants = 2,
 	.options = (struct tst_option[]) {
 		{"D:", &device, "Tty device. For example, /dev/tty[0-9]"},
 		{}
-- 
2.42.0


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

Re: [LTP] [PATCH v7 5/5] Extend ioctl02 to test termio and termios

[Martin Doucha]

Hi,
I'd strongly recommend merging this patch with the previous one. There's 
no point switching everything to termios only to add termio back in the 
next commit.

On 23. 10. 23 18:02, Marius Kittler wrote:
> Signed-off-by: Marius Kittler <mkittler@suse.de>
> ---
>   testcases/kernel/syscalls/ioctl/ioctl02.c | 170 +++++++++++++++++++---
>   1 file changed, 147 insertions(+), 23 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
> index 5111d7178..00d51b367 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl02.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
> @@ -8,22 +8,24 @@
>   /*\
>    * [Description]
>    *
> - * Testcase to test the TCGETS, and TCSETS ioctl implementations for
> - * the tty driver
> + * Testcase to test the TCGETA/TCGETS, and TCSETA/TCSETS ioctl
> + * implementations for the tty driver
>    *
>    * In this test, the parent and child open the parentty and the childtty
>    * respectively.  After opening the childtty the child flushes the stream
>    * and wakes the parent (thereby asking it to continue its testing). The
> - * parent, then starts the testing. It issues a TCGETS ioctl to get all
> - * the tty parameters. It then changes them to known values by issuing a
> - * TCSETS ioctl. Then the parent issues a TCGETS ioctl again and compares
> - * the received values with what it had set earlier. The test fails if
> - * TCGETS or TCSETS fails, or if the received values don't match those
> - * that were set. The parent does all the testing, the requirement of the
> - * child process is to moniter the testing done by the parent, and hence
> - * the child just waits for the parent.
> + * parent, then starts the testing. It issues a TCGETA/TCGETS ioctl to
> + * get all the tty parameters. It then changes them to known values by
> + * issuing a TCSETA/TCSETS ioctl. Then the parent issues a TCSETA/TCGETS
> + * ioctl again and compares the received values with what it had set
> + * earlier. The test fails if TCGETA/TCGETS or TCSETA/TCSETS fails, or if
> + * the received values don't match those that were set. The parent does
> + * all the testing, the requirement of the child process is to moniter
> + * the testing done by the parent, and hence the child just waits for the
> + * parent.
>    */
>   
> +#include <asm-generic/ioctls.h>

#including <asm/*> or <asm-generic/*> directly is generally a bad 
practice. You should #include <sys/ioctl.h> instead.

>   #include <stdio.h>
>   #include <fcntl.h>
>   #include <errno.h>
> @@ -38,23 +40,54 @@
>   #include "tst_test.h"
>   #include "tst_safe_macros.h"
>   
> -static struct termios termio, save_io;
> +static struct termio termio;
> +static struct termios termios, termios_bak;
>   
>   static char *parenttty, *childtty;
>   static int parentfd = -1, childfd = -1;
>   static int parentpid, childpid;
>   
>   static int do_child_setup(void);
> +static void prepare_termio(void);
> +static void preapre_termios(void);
>   static void run_ptest(void);
>   static void run_ctest(void);
> -static void chk_tty_parms(void);
> +static void chk_tty_parms_termio(void);
> +static void chk_tty_parms_termios(void);
>   static void setup(void);
>   static void cleanup(void);
>   
>   static char *device;
>   
> +static struct variant {
> +	const char *name;
> +	void *termio, *termio_bak;
> +	void (*prepare)(void);
> +	void (*check)(void);
> +	int tcget, tcset;
> +} variants[] = {
> +	{
> +		.name = "termio",
> +		.termio = &termio,
> +		.prepare = &prepare_termio,
> +		.check = &chk_tty_parms_termio,
> +		.tcget = TCGETA,
> +		.tcset = TCSETA,
> +	},
> +	{
> +		.name = "termios",
> +		.termio = &termios,
> +		.prepare = preapre_termios,
> +		.check = &chk_tty_parms_termios,
> +		.tcget = TCGETS,
> +		.tcset = TCSETS,
> +	},
> +};
> +
>   static void verify_ioctl(void)
>   {
> +	tst_res(TINFO, "Testing %s variant", variants[tst_variant].name);
> +
>   	parenttty = device;
>   	childtty = device;
>   
> @@ -80,11 +113,7 @@ static void verify_ioctl(void)
>   	TST_CHECKPOINT_WAKE(0);
>   }
>   
> -/*
> - * run_ptest() - setup the various termio structure values and issue
> - *		 the TCSETS ioctl call with the TEST macro.
> - */
> -static void run_ptest(void)
> +static void prepare_termio(void)
>   {
>   	/* Use "old" line discipline */
>   	termio.c_line = 0;
> @@ -110,12 +139,50 @@ static void run_ptest(void)
>   
>   	/* Set output modes. */
>   	termio.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
> +}
>   
> -	SAFE_IOCTL(parentfd, TCSETS, &termio);
> +static void preapre_termios(void)
> +{
> +	/* Use "old" line discipline */
> +	termios.c_line = 0;

You don't need to duplicate the whole prepare_termio() function. You can 
either initialize both structures at once:
termio.c_line = termios.c_line = 0;
termio.c_cflag = termios.c_cflag = ...;

of #define an init macro:
#define TERM_INIT_VALUES { .c_line = 0, .c_cflag = ... }
termio = (struct termio)TERM_INIT_VALUES;
termios = (struct termios)TERM_INIT_VALUES;

> +
> +	/* Set control modes */
> +	termios.c_cflag = B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL;
> +
> +	/* Set control chars. */
> +	for (int i = 0; i < NCC; i++) {
> +		if (i == VEOL2)
> +			continue;
> +		termios.c_cc[i] = CSTART;
> +	}
> +
> +	/* Set local modes. */
> +	termios.c_lflag =
> +	    ((unsigned short)(ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH));
> +
> +	/* Set input modes. */
> +	termios.c_iflag =
> +	    BRKINT | IGNPAR | INPCK | ISTRIP | ICRNL | IUCLC | IXON | IXANY |
> +	    IXOFF;
> +
> +	/* Set output modes. */
> +	termios.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
> +}
> +
> +/*
> + * run_ptest() - setup the various termio/termios structure values and issue
> + * the TCSETA/TCSETS ioctl call with the TEST macro.
> + */
> +static void run_ptest(void)
> +{
> +	struct variant *v = &variants[tst_variant];
> +
> +	v->prepare();
> +	SAFE_IOCTL(parentfd, v->tcset, v->termio);
>   
>   	/* Get termio and see if all parameters actually got set */
> -	SAFE_IOCTL(parentfd, TCGETS, &termio);
> -	chk_tty_parms();
> +	SAFE_IOCTL(parentfd, v->tcget, v->termio);
> +	v->check();
>   }
>   
>   static void run_ctest(void)
> @@ -125,7 +192,7 @@ static void run_ctest(void)
>   	SAFE_CLOSE(childfd);
>   }
>   
> -static void chk_tty_parms(void)
> +static void chk_tty_parms_termio(void)
>   {
>   	int i, flag = 0;
>   
> @@ -174,6 +241,62 @@ static void chk_tty_parms(void)
>   		flag++;
>   	}
>   
> +	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");
> +}
> +
> +static void chk_tty_parms_termios(void)
> +{
> +	int i, flag = 0;
> +
> +	if (termios.c_line != 0) {
> +		tst_res(TINFO, "line discipline has incorrect value %o",
> +			 termios.c_line);
> +		flag++;
> +	}

Some code duplication is unavoidable here but the value validation can 
be simplified:

1) Add another pair of termio+termios variables which will be 
initialized in setup() and passed only to SAFE_IOCTL(TCSETA/TCSETS).

2) memset() the existing termio+termios variables before passing them to 
SAFE_IOCTL(TCGETA/TCGETS). As is, the test can pass even if 
TCGETA/TCGETS does nothing because they store data into a structure that 
already contains the exact same values.

3) Define a macro to compare the "set" and "get" variables:
#define CMP_ATTR(tcexp, tcval, attr) \
	if ((tcval).attr != (tcexp).attr) { \
		tst_res(TINFO, #attr " has incorrect value %o", \
			 (tcval).attr); \
		flag++; \
	}

Then all attribtute checks except the loop can be reduced to one line.

> +
> +	for (i = 0; i < NCC; i++) {
> +		if (i == VEOL2) {
> +			if (!termios.c_cc[i]) {
> +				continue;
> +			} else {
> +				tst_res(TINFO, "control char %d has "
> +					 "incorrect value %d", i, termios.c_cc[i]);
> +				flag++;
> +				continue;
> +			}
> +		}
> +
> +		if (termios.c_cc[i] != CSTART) {
> +			tst_res(TINFO, "control char %d has incorrect "
> +				 "value %d.", i, termios.c_cc[i]);
> +			flag++;
> +		}
> +	}
> +
> +	if (termios.c_lflag != (ISIG | ICANON | XCASE | ECHO | ECHOE
> +		 | NOFLSH)) {
> +		tst_res(TINFO, "lflag has incorrect value. %o",
> +			 termios.c_lflag);
> +		flag++;
> +	}
> +
> +	if (termios.c_iflag != (BRKINT | IGNPAR | INPCK | ISTRIP
> +		 | ICRNL | IUCLC | IXON | IXANY | IXOFF)) {
> +		tst_res(TINFO, "iflag has incorrect value. %o",
> +			 termios.c_iflag);
> +		flag++;
> +	}
> +
> +	if (termios.c_oflag != (OPOST | OLCUC | ONLCR | ONOCR)) {
> +		tst_res(TINFO, "oflag has incorrect value. %o",
> +			 termios.c_oflag);
> +		flag++;
> +	}
> +
>   	if (flag != 0)
>   		tst_res(TFAIL, "TCGETS/TCSETS tests FAILED with "
>   				"%d %s", flag, flag > 1 ? "errors" : "error");
> @@ -202,7 +325,7 @@ static void setup(void)
>   	int fd = SAFE_OPEN(device, O_RDWR, 0777);
>   
>   	/* Save the current device information - to be restored in cleanup() */
> -	SAFE_IOCTL(fd, TCGETS, &save_io);
> +	SAFE_IOCTL(fd, TCGETS, &termios_bak);
>   
>   	/* Close the device */
>   	SAFE_CLOSE(fd);
> @@ -211,7 +334,7 @@ static void setup(void)
>   static void cleanup(void)
>   {
>   	if (parentfd >= 0) {
> -		SAFE_IOCTL(parentfd, TCSETS, &save_io);
> +		SAFE_IOCTL(parentfd, TCSETS, &termios_bak);
>   		SAFE_CLOSE(parentfd);
>   	}
>   }
> @@ -223,6 +346,7 @@ static struct tst_test test = {
>   	.setup = setup,
>   	.cleanup = cleanup,
>   	.test_all = verify_ioctl,
> +	.test_variants = 2,
>   	.options = (struct tst_option[]) {
>   		{"D:", &device, "Tty device. For example, /dev/tty[0-9]"},
>   		{}

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v7 5/5] Extend ioctl02 to test termio and termios

[Marius Kittler]

Am Dienstag, 24. Oktober 2023, 17:54:27 CEST schrieb Martin Doucha:

> You don't need to duplicate the whole prepare_termio() function. You can
> either initialize both structures at once:
> termio.c_line = termios.c_line = 0;
> termio.c_cflag = termios.c_cflag = ...;

That's not possible with all the assignments because the variables have 
different types (e.g. unsigned short vs. unsigned int). These different types 
are the reason why there are different structs in the first place. (Of course it 
would technically be possible but introducing implicit conversions and relying 
on these conversions not being too lossy is a bad idea in my opinion.)

> of #define an init macro:
> #define TERM_INIT_VALUES { .c_line = 0, .c_cflag = ... }
> termio = (struct termio)TERM_INIT_VALUES;
> termios = (struct termios)TERM_INIT_VALUES;
> 

That is rather ugly but likely the way to go then. And I assume I'd call this 
"combined" prepare function during the setup then.

> Some code duplication is unavoidable here but the value validation can
> be simplified:
> 
> 1) Add another pair of termio+termios variables which will be
> initialized in setup() and passed only to SAFE_IOCTL(TCSETA/TCSETS).
> 
> 2) memset() the existing termio+termios variables before passing them to
> SAFE_IOCTL(TCGETA/TCGETS). As is, the test can pass even if
> TCGETA/TCGETS does nothing because they store data into a structure that
> already contains the exact same values.
> 
> 3) Define a macro to compare the "set" and "get" variables:
> #define CMP_ATTR(tcexp, tcval, attr) \
> 	if ((tcval).attr != (tcexp).attr) { \
> 		tst_res(TINFO, #attr " has incorrect value %o", \
> 			 (tcval).attr); \
> 		flag++; \
> 	}
> 
> Then all attribtute checks except the loop can be reduced to one line.
> 

I guess that is the least ugly way to use macros, indeed. (Supposedly better 
than just wrapping the whole function body into a macro.)





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