[LTP] [PATCH v8 0/4] Improve ioctl02.c

[LTP] [PATCH v8 0/4] Improve ioctl02.c

[Marius Kittler]

I implemented again the changes requested for the first and last
patch.

That means I removed quite a few comments; I totally agree that
this code was over-commented. I kept a few comments (mainly in the
prepare function for the struct) because some struct members are
over-abbreviated (e.g. `c_cc`) so the comments actually do help
understanding the code. Some comments were also stating the
intention of the code which also seemed not completely useless.

I dropped the intermediate patch to use just termios.

In the last patch I decided to use the double-assignment
suggestion after all because the fields in termios are consistently
wider than the fields in termio so when just swapping the
assignment order this should be fine (there can never be a lossy
conversion). I also decided to make the loop a macro as well
because at this point we might as well go all-in with the macros.

Btw, you're sure you don't want to give C++ a try at some point :-)
(Just mentioning this because I really would have liked using C++
templates in this case and with C++ you can really pick what you
like and keep everything else in the usual C-style.)

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

 testcases/kernel/syscalls/ioctl/ioctl02.c | 551 +++++++---------------
 1 file changed, 171 insertions(+), 380 deletions(-)

-- 
2.42.0


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

[LTP] [PATCH v8 1/4] 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 | 414 +++++-----------------
 1 file changed, 89 insertions(+), 325 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index b4d4a3594..23a52151b 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -1,228 +1,87 @@
+// 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 <stdlib.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;
 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 do_child(void);
+static void run_ptest(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) {
+		do_child();
+		exit(EXIT_SUCCESS);
 	}
-	cleanup();
 
-	tst_exit();
-}
-
-static void do_child(void)
-{
-	childfd = do_child_setup();
-	if (childfd < 0)
-		_exit(1);
-	run_ctest();
-	_exit(0);
-}
+	TST_CHECKPOINT_WAIT(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_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 +89,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 +107,19 @@ 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)
-{
-	/*
-	 * 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;
-}
-
-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(TFAIL, "line discipline has incorrect value %o",
 			 termio.c_line);
 		flag++;
 	}
@@ -301,7 +132,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(TFAIL, "cflag has incorrect value. %o",
 			 termio.c_cflag);
 		flag++;
 	}
@@ -309,19 +140,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(TFAIL, "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(TFAIL, "control char %d has incorrect "
 				 "value %d.", i, termio.c_cc[i]);
 			flag++;
 		}
@@ -330,7 +160,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(TFAIL, "lflag has incorrect value. %o",
 			 termio.c_lflag);
 		flag++;
 	}
@@ -339,130 +169,64 @@ 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(TFAIL, "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(TFAIL, "oflag has incorrect value. %o",
 			 termio.c_oflag);
 		flag++;
 	}
 
 	if (!flag)
-		tst_resm(TINFO, "termio values are set as expected");
-
-	return flag;
+		tst_res(TPASS, "TCGETA/TCSETA tests");
 }
 
-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;
-}
-
-static int do_child_setup(void)
+static void do_child(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);
-
-	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;
-}
+	TST_CHECKPOINT_WAKE(0);
 
-static void help(void)
-{
-	printf("  -D <tty device> : for example, /dev/tty[0-9]\n");
+	TST_CHECKPOINT_WAIT(0);
+	tst_res(TINFO, "child: parent has finished testing");
+	SAFE_CLOSE(cfd);
 }
 
 static void setup(void)
 {
-	int fd;
-	struct sigaction act;
-
-	/* XXX: TERRNO required all over the place */
-	fd = SAFE_OPEN(NULL, devname, O_RDWR, 0777);
-
-	/* Save the current device information - to be restored in cleanup() */
-	SAFE_IOCTL(cleanup, 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);
+	if (!device)
+		tst_brk(TBROK, "You must specify a tty device with -D option");
 
-	act.sa_handler = (void *)sigusr1_handler;
-	act.sa_flags = 0;
-	(void)sigaction(SIGUSR1, &act, 0);
+	int fd = SAFE_OPEN(device, O_RDWR, 0777);
 
-	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_IOCTL(fd, TCGETA, &save_io);
+	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 v8 1/4] Refactor ioctl02.c to use the new test API

[Petr Vorel]

Hi Marius,

> * 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.

+1

Generally very nice cleanup.

NOTE: parentfd is not closed properly after use:

./ioctl02 -D /dev/tty0 -i1025
...
ioctl02.c:198: TINFO: child: parent has finished testing
ioctl02.c:184: TPASS: TCGETA/TCSETA tests
ioctl02.c:198: TINFO: child: parent has finished testing
ioctl02.c:189: TBROK: open(/dev/tty0,2,0000) failed: EMFILE (24)

You need to close it at the end of verify_ioctl()

@@ -74,6 +74,7 @@ static void verify_ioctl(void)
 	run_ptest();
 
 	TST_CHECKPOINT_WAKE(0);
+	SAFE_CLOSE(parentfd);
 }
 
 /*

---

There are #if 0 commented code with 2.6.24 comments. I doubt it's useful, please
delete it (the same problem I reported few months ago for your getxattr01.c rewrite).

With implementing these you can add:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few other comments below.

...
> +++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
> @@ -1,228 +1,87 @@
> +// 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>
nit: I would also add
* Copyright (c) Linux Test Project, 2001-2017

>   */

> -/*
> - * 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

nit: Sometimes we keep note about this developer (although the test is being
rewritten), thus I would omit him as well.

> - *
> - * 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
nit: "In this test" is ugly, but I'm not sure how to rewrite.

> + * 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.

nit: Creating paragraphs would make reading easier.

> @@ -330,7 +160,7 @@ static int chk_tty_parms(void)
>  	if (!
>  	    (termio.c_lflag
nit: this could have been on single line:
	if (!(termio.c_iflag

I know you will reformat it on 4th patch, but it's better to have cleanup in
single commit (in case the later changes get reverted).

>  	     && (ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH))) {
> -		tst_resm(TINFO, "lflag has incorrect value. %o",
> +		tst_res(TFAIL, "lflag has incorrect value. %o",
>  			 termio.c_lflag);
>  		flag++;
>  	}
> @@ -339,130 +169,64 @@ 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(TFAIL, "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(TFAIL, "oflag has incorrect value. %o",
>  			 termio.c_oflag);
>  		flag++;
>  	}

>  	if (!flag)
> -		tst_resm(TINFO, "termio values are set as expected");
> -
> -	return flag;
> +		tst_res(TPASS, "TCGETA/TCSETA tests");
>  }
...

> -	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);
2 is for flushing both Rx and Tx, right?
Maybe we could use TCIOFLUSH, right? At least it's documented in
include/uapi/asm-generic/termbits-common.h. TCIOFLUSH is in glibc, musl and
uclibc-ng thus safe to use.

> +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]"},

nit: I wonder if we could add in the setup() /dev/tty0 as a default device if -D
is not specified instead of TBROK (to be nice for developers which do the
debugging manually).

Kind regards,
Petr

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

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

[Petr Vorel]

Hi Marius,

> > -	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);
> 2 is for flushing both Rx and Tx, right?
> Maybe we could use TCIOFLUSH, right? At least it's documented in
> include/uapi/asm-generic/termbits-common.h. TCIOFLUSH is in glibc, musl and
> uclibc-ng thus safe to use.

And, more importantly, TCIOFLUSH is mentioned in man ioctl_tty(2) and
termios(3). Using constant is kind of self explaining.

Kind regards,
Petr

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

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

[Petr Vorel]

Hi Marius,

> There are #if 0 commented code with 2.6.24 comments. I doubt it's useful, please
> delete it (the same problem I reported few months ago for your getxattr01.c rewrite).

I see you have it in 3rd commit. I would merge squash it into the first one.
Martin has also suggested the same in v7.

Kind regards,
Petr

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

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

[Petr Vorel]

> Hi Marius,

> > There are #if 0 commented code with 2.6.24 comments. I doubt it's useful, please
> > delete it (the same problem I reported few months ago for your getxattr01.c rewrite).

> I see you have it in 3rd commit. I would merge squash it into the first one.

> Martin has also suggested the same in v7.
I'm sorry he did not. He actually added his RBT.

Kind regards,
Petr

> Kind regards,
> Petr

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

[LTP] [PATCH v8 2/4] 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 23a52151b..cfc081e9c 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -157,24 +157,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(TFAIL, "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(TFAIL, "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(TFAIL, "oflag has incorrect value. %o",
 			 termio.c_oflag);
 		flag++;
-- 
2.42.0


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

Re: [LTP] [PATCH v8 2/4] Make checks for termio flags more strict

[Petr Vorel]

Hi Marius,

...
> -	if (!
> -	    (termio.c_lflag
> -	     && (ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH))) {
> +	if (termio.c_lflag != (ISIG | ICANON | XCASE | ECHO | ECHOE
> +		 | NOFLSH)) {
Shouldn't this be using & ~() ?

	if ((termio.c_lflag & ~(ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH))) {

Also the other two.

Kind regards,
Petr

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

[LTP] [PATCH v8 3/4] 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 cfc081e9c..d5d19e1b9 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -123,20 +123,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(TFAIL, "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 v8 3/4] Remove disabled code in ioctl02.c

[Petr Vorel]

Hi Marius,

I'd squash it to the first commit (you can mention it in the commit message).

If it were a live code, I'd prefer removing it before the rewrite, but this was
not used for a long time.

Kind regards,
Petr

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

[LTP] [PATCH v8 4/4] Extend ioctl02 to test termio and termios

[Marius Kittler]

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

diff --git a/testcases/kernel/syscalls/ioctl/ioctl02.c b/testcases/kernel/syscalls/ioctl/ioctl02.c
index d5d19e1b9..365ce5a1f 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl02.c
@@ -8,26 +8,28 @@
 /*\
  * [Description]
  *
- * Testcase to test the TCGETA, and TCSETA 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 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.
+ * 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 <stdio.h>
 #include <stdlib.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <sys/ioctl.h>
 #include <sys/wait.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -39,22 +41,51 @@
 #include "tst_test.h"
 #include "tst_safe_macros.h"
 
-static struct termio termio, save_io;
+static struct termio termio, termio_exp;
+static struct termios termios, termios_exp, termios_bak;
 
 static char *parenttty, *childtty;
 static int parentfd = -1;
 static int parentpid, childpid;
 
 static void do_child(void);
+static void prepare_termio(void);
 static void run_ptest(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_exp, *termio_bak;
+	void (*check)(void);
+	int tcget, tcset;
+} variants[] = {
+	{
+		.name = "termio",
+		.termio = &termio,
+		.termio_exp = &termio_exp,
+		.check = &chk_tty_parms_termio,
+		.tcget = TCGETA,
+		.tcset = TCSETA,
+	},
+	{
+		.name = "termios",
+		.termio = &termios,
+		.termio_exp = &termios_exp,
+		.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;
 
@@ -76,97 +107,107 @@ static void verify_ioctl(void)
 	TST_CHECKPOINT_WAKE(0);
 }
 
-/*
- * run_ptest() - setup the various termio structure values and issue
- *		 the TCSETA ioctl call with the TEST macro.
- */
-static void run_ptest(void)
+static void prepare_termio(void)
 {
 	/* Use "old" line discipline */
-	termio.c_line = 0;
+	termios_exp.c_line = termio_exp.c_line = 0;
 
 	/* Set control modes */
-	termio.c_cflag = B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL;
+	termios_exp.c_cflag = termio_exp.c_cflag = B50 | CS7 | CREAD | PARENB | PARODD | CLOCAL;
 
 	/* Set control chars. */
 	for (int i = 0; i < NCC; i++) {
 		if (i == VEOL2)
 			continue;
-		termio.c_cc[i] = CSTART;
+		termios_exp.c_cc[i] = termio_exp.c_cc[i] = CSTART;
 	}
 
 	/* Set local modes. */
-	termio.c_lflag =
+	termios_exp.c_lflag = termio_exp.c_lflag =
 	    ((unsigned short)(ISIG | ICANON | XCASE | ECHO | ECHOE | NOFLSH));
 
 	/* Set input modes. */
-	termio.c_iflag =
+	termios_exp.c_iflag = termio_exp.c_iflag =
 	    BRKINT | IGNPAR | INPCK | ISTRIP | ICRNL | IUCLC | IXON | IXANY |
 	    IXOFF;
 
 	/* Set output modes. */
-	termio.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
-
-	SAFE_IOCTL(parentfd, TCSETA, &termio);
+	termios_exp.c_oflag = termio_exp.c_oflag = OPOST | OLCUC | ONLCR | ONOCR;
 
-	/* Get termio and see if all parameters actually got set */
-	SAFE_IOCTL(parentfd, TCGETA, &termio);
-	chk_tty_parms();
+	/* Init termio/termios structures used to check if all params got set */
+	memset(&termio, 0, sizeof(termio));
+	memset(&termios, 0, sizeof(termios));
 }
 
-static void chk_tty_parms(void)
+/*
+ * 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)
 {
-	int i, flag = 0;
+	struct variant *v = &variants[tst_variant];
 
-	if (termio.c_line != 0) {
-		tst_res(TFAIL, "line discipline has incorrect value %o",
-			 termio.c_line);
-		flag++;
-	}
-
-	for (i = 0; i < NCC; i++) {
-		if (i == VEOL2) {
-			if (!termio.c_cc[i]) {
-				continue;
-			} else {
-				tst_res(TFAIL, "control char %d has "
-					 "incorrect value %d", i, termio.c_cc[i]);
-				flag++;
-				continue;
-			}
-		}
-
-		if (termio.c_cc[i] != CSTART) {
-			tst_res(TFAIL, "control char %d has incorrect "
-				 "value %d.", i, termio.c_cc[i]);
-			flag++;
-		}
-	}
+	SAFE_IOCTL(parentfd, v->tcset, v->termio_exp);
 
-	if (termio.c_lflag != (ISIG | ICANON | XCASE | ECHO | ECHOE
-		 | NOFLSH)) {
-		tst_res(TFAIL, "lflag has incorrect value. %o",
-			 termio.c_lflag);
-		flag++;
-	}
+	/* Get termio and see if all parameters actually got set */
+	SAFE_IOCTL(parentfd, v->tcget, v->termio);
+	v->check();
+}
 
-	if (termio.c_iflag != (BRKINT | IGNPAR | INPCK | ISTRIP
-		 | ICRNL | IUCLC | IXON | IXANY | IXOFF)) {
-		tst_res(TFAIL, "iflag has incorrect value. %o",
-			 termio.c_iflag);
-		flag++;
+#define CMP_ATTR(tcexp, tcval, attr) \
+	do { \
+		if ((tcval).attr != (tcexp).attr) { \
+			tst_res(TINFO, #attr " has incorrect value %o", \
+				(tcval).attr); \
+			flag++; \
+		} \
+	} while (0)
+
+#define CECK_CONTROL_CHARS(tcval) \
+	for (i = 0; i < NCC; i++) { \
+		if (i == VEOL2) { \
+			if (!(tcval).c_cc[i]) { \
+				continue; \
+			} else { \
+				tst_res(TFAIL, "control char %d has " \
+					 "incorrect value %d", i, (tcval).c_cc[i]); \
+				flag++; \
+				continue; \
+			} \
+		} \
+		if ((tcval).c_cc[i] != CSTART) { \
+			tst_res(TFAIL, "control char %d has incorrect " \
+				 "value %d.", i, (tcval).c_cc[i]); \
+			flag++; \
+		} \
 	}
 
-	if (termio.c_oflag != (OPOST | OLCUC | ONLCR | ONOCR)) {
-		tst_res(TFAIL, "oflag has incorrect value. %o",
-			 termio.c_oflag);
-		flag++;
-	}
+static void chk_tty_parms_termio(void)
+{
+	int i, flag = 0;
 
+	CMP_ATTR(termio_exp, termio, c_line);
+	CECK_CONTROL_CHARS(termio);
+	CMP_ATTR(termio_exp, termio, c_lflag);
+	CMP_ATTR(termio_exp, termio, c_iflag);
+	CMP_ATTR(termio_exp, termio, c_oflag);
 	if (!flag)
 		tst_res(TPASS, "TCGETA/TCSETA tests");
 }
 
+static void chk_tty_parms_termios(void)
+{
+	int i, flag = 0;
+
+	CMP_ATTR(termios_exp, termios, c_line);
+	CECK_CONTROL_CHARS(termios);
+	CMP_ATTR(termios_exp, termios, c_lflag);
+	CMP_ATTR(termios_exp, termios, c_iflag);
+	CMP_ATTR(termios_exp, termios, c_oflag);
+	if (!flag)
+		tst_res(TPASS, "TCGETS/TCSETS tests");
+}
+
 static void do_child(void)
 {
 	int cfd = SAFE_OPEN(childtty, O_RDWR, 0777);
@@ -189,14 +230,16 @@ static void setup(void)
 
 	int fd = SAFE_OPEN(device, O_RDWR, 0777);
 
-	SAFE_IOCTL(fd, TCGETA, &save_io);
+	SAFE_IOCTL(fd, TCGETS, &termios_bak);
 	SAFE_CLOSE(fd);
+
+	prepare_termio();
 }
 
 static void cleanup(void)
 {
 	if (parentfd >= 0) {
-		SAFE_IOCTL(parentfd, TCSETA, &save_io);
+		SAFE_IOCTL(parentfd, TCSETS, &termios_bak);
 		SAFE_CLOSE(parentfd);
 	}
 }
@@ -208,6 +251,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 v8 4/4] Extend ioctl02 to test termio and termios

[Petr Vorel]

Hi Marius

...
> -	if (termio.c_iflag != (BRKINT | IGNPAR | INPCK | ISTRIP
> -		 | ICRNL | IUCLC | IXON | IXANY | IXOFF)) {
> -		tst_res(TFAIL, "iflag has incorrect value. %o",
> -			 termio.c_iflag);
> -		flag++;

> +#define CMP_ATTR(tcexp, tcval, attr) \
> +	do { \
> +		if ((tcval).attr != (tcexp).attr) { \
> +			tst_res(TINFO, #attr " has incorrect value %o", \
> +				(tcval).attr); \
> +			flag++; \
> +		} \
> +	} while (0)
> +
> +#define CECK_CONTROL_CHARS(tcval) \
> +	for (i = 0; i < NCC; i++) { \
> +		if (i == VEOL2) { \
> +			if (!(tcval).c_cc[i]) { \
> +				continue; \
> +			} else { \
> +				tst_res(TFAIL, "control char %d has " \
> +					 "incorrect value %d", i, (tcval).c_cc[i]); \
> +				flag++; \
> +				continue; \
> +			} \
> +		} \
> +		if ((tcval).c_cc[i] != CSTART) { \
> +			tst_res(TFAIL, "control char %d has incorrect " \
> +				 "value %d.", i, (tcval).c_cc[i]); \
> +			flag++; \
> +		} \
>  	}

Could be this written as a function? Or what is the benefit of it?  Because
readability suffers. (We prefer avoid macros, tst_test_macros.h is the exception
due using kernel syscalls, but readability also suffers).

I know you mentioned C++ in cover letter, but please no C++ :).

Kind regards,
Petr

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

Re: [LTP] [PATCH v8 4/4] Extend ioctl02 to test termio and termios

[iob via ltp]

Petr Vorel <pvorel@suse.cz> writes:

> Hi Marius
>
> ...
>> -	if (termio.c_iflag != (BRKINT | IGNPAR | INPCK | ISTRIP
>> -		 | ICRNL | IUCLC | IXON | IXANY | IXOFF)) {
>> -		tst_res(TFAIL, "iflag has incorrect value. %o",
>> -			 termio.c_iflag);
>> -		flag++;
>
>> +#define CMP_ATTR(tcexp, tcval, attr) \
>> +	do { \
>> +		if ((tcval).attr != (tcexp).attr) { \
>> +			tst_res(TINFO, #attr " has incorrect value %o", \
>> +				(tcval).attr); \
>> +			flag++; \
>> +		} \
>> +	} while (0)
>> +
>> +#define CECK_CONTROL_CHARS(tcval) \
Was this meant to named as CHECK_CONTROL_CHARS?


>> +	for (i = 0; i < NCC; i++) { \
>> +		if (i == VEOL2) { \
>> +			if (!(tcval).c_cc[i]) { \
>> +				continue; \
>> +			} else { \
>> +				tst_res(TFAIL, "control char %d has " \
>> +					 "incorrect value %d", i, (tcval).c_cc[i]); \
>> +				flag++; \
>> +				continue; \
>> +			} \
>> +		} \
>> +		if ((tcval).c_cc[i] != CSTART) { \
>> +			tst_res(TFAIL, "control char %d has incorrect " \
>> +				 "value %d.", i, (tcval).c_cc[i]); \
>> +			flag++; \
>> +		} \
>>  	}
>
> Could be this written as a function? Or what is the benefit of it?  Because
> readability suffers. (We prefer avoid macros, tst_test_macros.h is the exception
> due using kernel syscalls, but readability also suffers).
>
> I know you mentioned C++ in cover letter, but please no C++ :).
>
> Kind regards,
> Petr


-- 
Sent with my mu4e

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

Re: [LTP] [PATCH v8 4/4] Extend ioctl02 to test termio and termios

[Petr Vorel]

Hi all,

...
> >> +#define CMP_ATTR(tcexp, tcval, attr) \
> >> +	do { \
> >> +		if ((tcval).attr != (tcexp).attr) { \
> >> +			tst_res(TINFO, #attr " has incorrect value %o", \
> >> +				(tcval).attr); \
> >> +			flag++; \
> >> +		} \
> >> +	} while (0)
> >> +
> >> +#define CECK_CONTROL_CHARS(tcval) \
> Was this meant to named as CHECK_CONTROL_CHARS?

+1

Kind regards,
Petr

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

Re: [LTP] [PATCH v8 0/4] Improve ioctl02.c

[Petr Vorel]

Hi Marius,

> I implemented again the changes requested for the first and last
> patch.

+1

> That means I removed quite a few comments; I totally agree that
> this code was over-commented. I kept a few comments (mainly in the
> prepare function for the struct) because some struct members are
> over-abbreviated (e.g. `c_cc`) so the comments actually do help
> understanding the code. Some comments were also stating the
> intention of the code which also seemed not completely useless.

+1

> I dropped the intermediate patch to use just termios.

I don't know why, but that was for sure discussed in some previous versions.
Or do you want to get back to it after this effort gets merged?

> In the last patch I decided to use the double-assignment
> suggestion after all because the fields in termios are consistently
> wider than the fields in termio so when just swapping the
> assignment order this should be fine (there can never be a lossy
> conversion). I also decided to make the loop a macro as well
> because at this point we might as well go all-in with the macros.

We usually prefer functions, which are more readable. But I'll comment that more
at the 4th patch.

> Btw, you're sure you don't want to give C++ a try at some point :-)
> (Just mentioning this because I really would have liked using C++
> templates in this case and with C++ you can really pick what you
> like and keep everything else in the usual C-style.)

I don't think either C++ templates for LTP would be a good idea.
The code is very simple to include any templating system.
Also testing (g)libc headers should be IMHO done with plain C.

Kind regards,
Petr

> Marius Kittler (4):
>   Refactor ioctl02.c to use the new test API
>   Make checks for termio flags more strict
>   Remove disabled code in ioctl02.c
>   Extend ioctl02 to test termio and termios

>  testcases/kernel/syscalls/ioctl/ioctl02.c | 551 +++++++---------------
>  1 file changed, 171 insertions(+), 380 deletions(-)

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