| Message ID | 20230207131714.2500-2-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ioctl01: device check, enable | expand |
Hi Petr, On Tue, Feb 7, 2023 at 9:17 PM Petr Vorel <pvorel@suse.cz> wrote: > Saves user to specify it when run manually. > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > testcases/kernel/syscalls/ioctl/ioctl01.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl01.c > b/testcases/kernel/syscalls/ioctl/ioctl01.c > index 1be38e79d3..cb184aee40 100644 > --- a/testcases/kernel/syscalls/ioctl/ioctl01.c > +++ b/testcases/kernel/syscalls/ioctl/ioctl01.c > @@ -27,6 +27,7 @@ > #include "lapi/ioctl.h" > > #define INVAL_IOCTL 9999999 > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > Hidden the device path parameter is a good idea. But maybe can we add a function to find available char devices instead of using the tty0 as default? In that function, we do the S_ISCHR() check and return the valid path of it. Then the rest test (e.g. ioctl02) can make use of it but not set the specified device as well. WDYT? > > static int fd, fd_file; > static int bfd = -1; > @@ -70,7 +71,9 @@ static void verify_ioctl(unsigned int i) > static void setup(void) > { > if (!device) > - tst_brk(TBROK, "You must specify a tty device with -D > option"); > + device = DEFAULT_TTY_DEVICE; > + > + tst_res(TINFO, "Using device '%s'", device); > > fd = SAFE_OPEN(device, O_RDWR, 0777); > fd_file = SAFE_OPEN("x", O_CREAT, 0777); > -- > 2.39.1 > >
Hi Li, > > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > Hidden the device path parameter is a good idea. > But maybe can we add a function to find available char devices instead > of using the tty0 as default? In that function, we do the S_ISCHR() check > and return the valid path of it. Then the rest test (e.g. ioctl02) can make > use of it but not set the specified device as well. WDYT? FYI I'm using S_ISCHR() in other patches, which check if device can be used. Implementing search looks like a good idea. Are useful files any /dev/tty* (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file or include other paths? Kind regards, Petr
> Hi Li, > > > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > > Hidden the device path parameter is a good idea. > > But maybe can we add a function to find available char devices instead > > of using the tty0 as default? In that function, we do the S_ISCHR() check > > and return the valid path of it. Then the rest test (e.g. ioctl02) can make > > use of it but not set the specified device as well. WDYT? > FYI I'm using S_ISCHR() in other patches, which check if device can be used. > Implementing search looks like a good idea. Are useful files any /dev/tty* > (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file > or include other paths? I also wonder if we still want to keep -D parameter (i.e. allow tester to pass a file). Kind regards, Petr > Kind regards, > Petr
On Wed, Feb 8, 2023 at 9:54 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > > > Hidden the device path parameter is a good idea. > > > But maybe can we add a function to find available char devices instead > > of using the tty0 as default? In that function, we do the S_ISCHR() check > > and return the valid path of it. Then the rest test (e.g. ioctl02) can > make > > use of it but not set the specified device as well. WDYT? > > FYI I'm using S_ISCHR() in other patches, which check if device can be > used. > Implementing search looks like a good idea. Are useful files any /dev/tty* > (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file > or include other paths? > It seems not all char devices can be used here. The /dev/tty*Num* should be the first choice. I tried some of them and only /dev/tty0-N works well, maybe we can just make use of them to avoid no tty0 file failure should be enough. Because my only concern about the hard coding is that "/dev/tty0" is non-exist. > > Kind regards, > Petr > >
On Wed, Feb 8, 2023 at 10:13 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, > > > > > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > > > > Hidden the device path parameter is a good idea. > > > > But maybe can we add a function to find available char devices instead > > > of using the tty0 as default? In that function, we do the S_ISCHR() > check > > > and return the valid path of it. Then the rest test (e.g. ioctl02) can > make > > > use of it but not set the specified device as well. WDYT? > > > FYI I'm using S_ISCHR() in other patches, which check if device can be > used. > > Implementing search looks like a good idea. Are useful files any > /dev/tty* > > (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file > > or include other paths? > > I also wonder if we still want to keep -D parameter (i.e. allow tester to > pass a > file). > I think there is no necessary keep "-D" parameter since this ioctl01 is to check errno of ioctl(2) syscall, it might meaningless to specify a different char device.
... > > FYI I'm using S_ISCHR() in other patches, which check if device can be > > used. > > Implementing search looks like a good idea. Are useful files any /dev/tty* > > (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file > > or include other paths? > It seems not all char devices can be used here. > The /dev/tty*Num* should be the first choice. +1 I'd also use /dev/tty (in case there is no tty0). > I tried some of them and only /dev/tty0-N works well, > maybe we can just make use of them to avoid no tty0 > file failure should be enough. Because my only concern > about the hard coding is that "/dev/tty0" is non-exist. FYI On my system ioctl01 fail on /dev/ttyS0-N (EIO instead of other expected errors). The rest /dev/tty, /dev/tty0-N, /dev/ttyACM0-N work. /dev/ttyACM0-N are just on one of my system, let's not use it. Kind regards, Petr
... > > I also wonder if we still want to keep -D parameter (i.e. allow tester to > > pass a > > file). > I think there is no necessary keep "-D" parameter since this ioctl01 > is to check errno of ioctl(2) syscall, it might meaningless to specify a > different char device. Thx! That makes checking code slightly simpler. Kind regards, Petr
Petr Vorel <pvorel@suse.cz> writes: > Hi Li, > >> > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > >> Hidden the device path parameter is a good idea. > >> But maybe can we add a function to find available char devices >instead There is already something like this built into the kernel; you can create a PTY on demand with /dev/ptmx. See the kernel/pty tests. >> of using the tty0 as default? In that function, we do the S_ISCHR() check >> and return the valid path of it. Then the rest test (e.g. ioctl02) can make >> use of it but not set the specified device as well. WDYT? > > FYI I'm using S_ISCHR() in other patches, which check if device can be used. > Implementing search looks like a good idea. Are useful files any /dev/tty* > (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file > or include other paths? These are real serial devices except /dev/tty which could be a real device or a pty IIUC. Same goes for /dev/hvc[0-9] and possibly some others. I'm going to put the patch set to changes requested because /dev/tty0 is the current virtual console. It seems the test just overwrites the permissions and starts sending ioctls to it. I don't know if this is safe and probably it's no different from creating a pty. > > Kind regards, > Petr
> Petr Vorel <pvorel@suse.cz> writes: > > Hi Li, > >> > +#define DEFAULT_TTY_DEVICE "/dev/tty0" > >> Hidden the device path parameter is a good idea. > >> But maybe can we add a function to find available char devices > >instead > There is already something like this built into the kernel; you can > create a PTY on demand with /dev/ptmx. > See the kernel/pty tests. Thanks for a tip! According to man ptmx(4) the file /dev/ptmx creates pseudoterminal (PTYs) in /dev/pts directory (e.g. /dev/pts/0). > >> of using the tty0 as default? In that function, we do the S_ISCHR() check > >> and return the valid path of it. Then the rest test (e.g. ioctl02) can make > >> use of it but not set the specified device as well. WDYT? > > FYI I'm using S_ISCHR() in other patches, which check if device can be used. > > Implementing search looks like a good idea. Are useful files any /dev/tty* > > (including /dev/tty, /dev/ttyACM0, /dev/ttyS0) or should I avoid any file > > or include other paths? BTW /dev/ttySN are UART serial port TTY (it was familiar to me, although I nowadays use /dev/ttyUSB0 :)). > These are real serial devices except /dev/tty which could be a real > device or a pty IIUC. Same goes for /dev/hvc[0-9] and possibly some > others. > I'm going to put the patch set to changes requested because /dev/tty0 is > the current virtual console. It seems the test just overwrites the > permissions and starts sending ioctls to it. Sure. > I don't know if this is safe and probably it's no different from > creating a pty. Kernel doc [1] mentions more about the devices. I suppose it does not matter for ioctl01 purpose, which TTY device we use, so I'll use your suggestion. Kind regards, Petr [1] https://www.kernel.org/doc/html/latest/admin-guide/devices.html > > Kind regards, > > Petr
diff --git a/testcases/kernel/syscalls/ioctl/ioctl01.c b/testcases/kernel/syscalls/ioctl/ioctl01.c index 1be38e79d3..cb184aee40 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl01.c +++ b/testcases/kernel/syscalls/ioctl/ioctl01.c @@ -27,6 +27,7 @@ #include "lapi/ioctl.h" #define INVAL_IOCTL 9999999 +#define DEFAULT_TTY_DEVICE "/dev/tty0" static int fd, fd_file; static int bfd = -1; @@ -70,7 +71,9 @@ static void verify_ioctl(unsigned int i) static void setup(void) { if (!device) - tst_brk(TBROK, "You must specify a tty device with -D option"); + device = DEFAULT_TTY_DEVICE; + + tst_res(TINFO, "Using device '%s'", device); fd = SAFE_OPEN(device, O_RDWR, 0777); fd_file = SAFE_OPEN("x", O_CREAT, 0777);
Saves user to specify it when run manually. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- testcases/kernel/syscalls/ioctl/ioctl01.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)