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

[Petr Vorel <pvorel@suse.cz>]

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