diff mbox series

[V7,14/19] syscalls/select6: Add support for time64 tests

Message ID 883a188e83c201b23074bf0ac974b41d89d818c7.1593152309.git.viresh.kumar@linaro.org
State Changes Requested
Headers show
Series Syscalls: Add support for time64 variants | expand

Commit Message

Viresh Kumar June 26, 2020, 6:22 a.m. UTC
This adds support for time64 tests to the existing select6() syscall
tests.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 testcases/kernel/syscalls/select/select_var.h | 25 ++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

Cyril Hrubis July 27, 2020, 9:40 a.m. UTC | #1
Hi!
> This adds support for time64 tests to the existing select6() syscall
> tests.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  testcases/kernel/syscalls/select/select_var.h | 25 ++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/select/select_var.h b/testcases/kernel/syscalls/select/select_var.h
> index b19a1d1bf085..2c7604807cf6 100644
> --- a/testcases/kernel/syscalls/select/select_var.h
> +++ b/testcases/kernel/syscalls/select/select_var.h
> @@ -6,6 +6,7 @@
>  #define SELECT_VAR__
>  
>  #include "lapi/syscalls.h"
> +#include "tst_timer.h"
>  
>  struct compat_sel_arg_struct {
>  	long _n;
> @@ -38,7 +39,7 @@ static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
>  	}
>  	case 2: {
>  		int ret;
> -		struct timespec ts = {
> +		struct __kernel_old_timespec ts = {
>  			.tv_sec = timeout->tv_sec,
>  			.tv_nsec = timeout->tv_usec * 1000,
>  		};

I'm a bit lost here, should we actually pass the __kernel_old_timespec
to all the tst_syscall() fuctions here?

I guess that the only function that would take the argument as struct
timeval is the select() glibc function, or do I miss something?

Also this change should be ideally done in a separate patch from the
second half that adds the __NR_pselect6_time64.

> @@ -47,7 +48,22 @@ static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
>  		timeout->tv_usec = ts.tv_nsec / 1000;
>  		return ret;
>  	}
> -	case 3:
> +	case 3: {
> +		int ret = 0;
> +#if (__NR_clock_settime64 != __LTP__NR_INVALID_SYSCALL)
            ^
	    __NR_pselect6_time64 ?

> +		struct __kernel_timespec ts = {
> +			.tv_sec = timeout->tv_sec,
> +			.tv_nsec = timeout->tv_usec * 1000,
> +		};
> +		ret = tst_syscall(__NR_pselect6_time64, nfds, readfds, writefds, exceptfds, &ts, NULL);
> +		timeout->tv_sec = ts.tv_sec;
> +		timeout->tv_usec = ts.tv_nsec / 1000;
> +#else
> +		tst_brk(TCONF, "__NR_pselect6 time64 variant not supported");
> +#endif
> +		return ret;
> +	}
> +	case 4:
>  #ifdef __NR__newselect
>  		return tst_syscall(__NR__newselect, nfds, readfds, writefds, exceptfds, timeout);
>  #else
> @@ -72,11 +88,14 @@ static void select_info(void)
>  		tst_res(TINFO, "Testing SYS_pselect6 syscall");
>  	break;
>  	case 3:
> +		tst_res(TINFO, "Testing SYS_pselect6 time64 syscall");
> +	break;
> +	case 4:
>  		tst_res(TINFO, "Testing SYS__newselect syscall");
>  	break;
>  	}
>  }
>  
> -#define TEST_VARIANTS 4
> +#define TEST_VARIANTS 5

Also lastly but not least we should clean up the rest of the select
tests and add support for the different variants there as well.

Looking at them these are just copy&paste of the same test with a
different fds, we can easily merge them into a single test.

And the coverate in these tests is a bit lacking, we do not have a
single tests that would send a data over a pipe to a fd select is
watching and check that select was woken up by that. There is no such
test in the pselect/ directory either.
Viresh Kumar July 28, 2020, 7:23 a.m. UTC | #2
On 27-07-20, 11:40, Cyril Hrubis wrote:
> > @@ -38,7 +39,7 @@ static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
> >  	}
> >  	case 2: {
> >  		int ret;
> > -		struct timespec ts = {
> > +		struct __kernel_old_timespec ts = {
> >  			.tv_sec = timeout->tv_sec,
> >  			.tv_nsec = timeout->tv_usec * 1000,
> >  		};
> 
> I'm a bit lost here, should we actually pass the __kernel_old_timespec
> to all the tst_syscall() fuctions here?

select, pselect6, pselect6_time64, and newselect, all have different
requirements, some take timespec and others take timeval.

Though after looking again at kernel sources I feel pselect6 may need
__kernel_timespec instead of __kernel_old_timespec, which is different than what
we did with other syscalls.

Arnd, can you confirm this please ?

> I guess that the only function that would take the argument as struct
> timeval is the select() glibc function, or do I miss something?

select in kernel also takes old timeval.

> > @@ -47,7 +48,22 @@ static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
> >  		timeout->tv_usec = ts.tv_nsec / 1000;
> >  		return ret;
> >  	}
> > -	case 3:
> > +	case 3: {
> > +		int ret = 0;
> > +#if (__NR_clock_settime64 != __LTP__NR_INVALID_SYSCALL)
>             ^
> 	    __NR_pselect6_time64 ?
> 
> > +		struct __kernel_timespec ts = {
> > +			.tv_sec = timeout->tv_sec,
> > +			.tv_nsec = timeout->tv_usec * 1000,
> > +		};
> > +		ret = tst_syscall(__NR_pselect6_time64, nfds, readfds, writefds, exceptfds, &ts, NULL);
> > +		timeout->tv_sec = ts.tv_sec;
> > +		timeout->tv_usec = ts.tv_nsec / 1000;
> > +#else
> > +		tst_brk(TCONF, "__NR_pselect6 time64 variant not supported");
> > +#endif
> > +		return ret;
> > +	}
> > +	case 4:
> >  #ifdef __NR__newselect
> >  		return tst_syscall(__NR__newselect, nfds, readfds, writefds, exceptfds, timeout);
> >  #else
> > @@ -72,11 +88,14 @@ static void select_info(void)
> >  		tst_res(TINFO, "Testing SYS_pselect6 syscall");
> >  	break;
> >  	case 3:
> > +		tst_res(TINFO, "Testing SYS_pselect6 time64 syscall");
> > +	break;
> > +	case 4:
> >  		tst_res(TINFO, "Testing SYS__newselect syscall");
> >  	break;
> >  	}
> >  }
> >  
> > -#define TEST_VARIANTS 4
> > +#define TEST_VARIANTS 5
> 
> Also lastly but not least we should clean up the rest of the select
> tests and add support for the different variants there as well.

Maybe not. IIUC only pselect6 got changed to adapt to different timespec
structures and the other ones aren't.

Arnd: Can you confirm this as well ?

> Looking at them these are just copy&paste of the same test with a
> different fds, we can easily merge them into a single test.
> 
> And the coverate in these tests is a bit lacking, we do not have a
> single tests that would send a data over a pipe to a fd select is
> watching and check that select was woken up by that. There is no such
> test in the pselect/ directory either.

Hmm, I will look at that separately then.
Arnd Bergmann July 28, 2020, 8:02 a.m. UTC | #3
On Tue, Jul 28, 2020 at 9:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-07-20, 11:40, Cyril Hrubis wrote:
> > > @@ -38,7 +39,7 @@ static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
> > >     }
> > >     case 2: {
> > >             int ret;
> > > -           struct timespec ts = {
> > > +           struct __kernel_old_timespec ts = {
> > >                     .tv_sec = timeout->tv_sec,
> > >                     .tv_nsec = timeout->tv_usec * 1000,
> > >             };
> >
> > I'm a bit lost here, should we actually pass the __kernel_old_timespec
> > to all the tst_syscall() fuctions here?
>
> select, pselect6, pselect6_time64, and newselect, all have different
> requirements, some take timespec and others take timeval.
>
> Though after looking again at kernel sources I feel pselect6 may need
> __kernel_timespec instead of __kernel_old_timespec, which is different than what
> we did with other syscalls.
>
> Arnd, can you confirm this please ?

On the kernel side, there is '__NR_pselect6', which refers to the syscall
that takes a 'struct __kernel_old_timespec'., while 32-bit architectures
have another syscall with the number __NR_pselect6_time64 that
takes a 'struct __kernel_timespec'.

The kernel implementation internally uses sys_pselect6_time32()
for the entry point that takes a 32-bit time_t (__NR_pselect6 on
32-bit architectures), and sys_pselect6() for the entry point
that passes a 64-bit time_t (__NR_pselect6 on 64-bit architectures
or __NR_pselect6_time64 on 32-bit architectures).

> > I guess that the only function that would take the argument as struct
> > timeval is the select() glibc function, or do I miss something?
>
> select in kernel also takes old timeval.

Correct, though most modern architectures do not implement this
and only provide pselect6 (and pselect6_time64).

> > > @@ -72,11 +88,14 @@ static void select_info(void)
> > >             tst_res(TINFO, "Testing SYS_pselect6 syscall");
> > >     break;
> > >     case 3:
> > > +           tst_res(TINFO, "Testing SYS_pselect6 time64 syscall");
> > > +   break;
> > > +   case 4:
> > >             tst_res(TINFO, "Testing SYS__newselect syscall");
> > >     break;
> > >     }
> > >  }
> > >
> > > -#define TEST_VARIANTS 4
> > > +#define TEST_VARIANTS 5
> >
> > Also lastly but not least we should clean up the rest of the select
> > tests and add support for the different variants there as well.
>
> Maybe not. IIUC only pselect6 got changed to adapt to different timespec
> structures and the other ones aren't.
>
> Arnd: Can you confirm this as well ?

Yes, this is correct. __NR__newselect (sys_select) and __NR_select
(sys_old_select or sys_select) are both considered obsolete and only
provided on older architectures for backwards compatibility with old libc
versions.

       Arnd
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/select/select_var.h b/testcases/kernel/syscalls/select/select_var.h
index b19a1d1bf085..2c7604807cf6 100644
--- a/testcases/kernel/syscalls/select/select_var.h
+++ b/testcases/kernel/syscalls/select/select_var.h
@@ -6,6 +6,7 @@ 
 #define SELECT_VAR__
 
 #include "lapi/syscalls.h"
+#include "tst_timer.h"
 
 struct compat_sel_arg_struct {
 	long _n;
@@ -38,7 +39,7 @@  static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
 	}
 	case 2: {
 		int ret;
-		struct timespec ts = {
+		struct __kernel_old_timespec ts = {
 			.tv_sec = timeout->tv_sec,
 			.tv_nsec = timeout->tv_usec * 1000,
 		};
@@ -47,7 +48,22 @@  static int do_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *except
 		timeout->tv_usec = ts.tv_nsec / 1000;
 		return ret;
 	}
-	case 3:
+	case 3: {
+		int ret = 0;
+#if (__NR_clock_settime64 != __LTP__NR_INVALID_SYSCALL)
+		struct __kernel_timespec ts = {
+			.tv_sec = timeout->tv_sec,
+			.tv_nsec = timeout->tv_usec * 1000,
+		};
+		ret = tst_syscall(__NR_pselect6_time64, nfds, readfds, writefds, exceptfds, &ts, NULL);
+		timeout->tv_sec = ts.tv_sec;
+		timeout->tv_usec = ts.tv_nsec / 1000;
+#else
+		tst_brk(TCONF, "__NR_pselect6 time64 variant not supported");
+#endif
+		return ret;
+	}
+	case 4:
 #ifdef __NR__newselect
 		return tst_syscall(__NR__newselect, nfds, readfds, writefds, exceptfds, timeout);
 #else
@@ -72,11 +88,14 @@  static void select_info(void)
 		tst_res(TINFO, "Testing SYS_pselect6 syscall");
 	break;
 	case 3:
+		tst_res(TINFO, "Testing SYS_pselect6 time64 syscall");
+	break;
+	case 4:
 		tst_res(TINFO, "Testing SYS__newselect syscall");
 	break;
 	}
 }
 
-#define TEST_VARIANTS 4
+#define TEST_VARIANTS 5
 
 #endif /* SELECT_VAR__ */