[LTP] [PATCH v8 1/4] Refactor ioctl02.c to use the new test API
Petr Vorel
pvorel@suse.cz
Thu Oct 26 00:06:05 CEST 2023
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
More information about the ltp
mailing list