Re: [LTP] [PATCH] syscalls/setitimer: Pass the kernel-defined struct __kernel_old_itimerval to sys_setitimer().

[Petr Vorel <pvorel@suse.cz>]

Hi all,

> +++ b/configure.ac
> @@ -219,7 +219,8 @@ AC_CHECK_TYPES([struct xt_entry_match, struct xt_entry_target],,,[
>  ])

>  AC_CHECK_TYPES([struct __kernel_old_timeval, struct __kernel_old_timespec, struct __kernel_timespec,
> -                struct __kernel_old_itimerspec, struct __kernel_itimerspec],,,[#include <sys/socket.h>])
> +                struct __kernel_old_itimerspec, struct __kernel_itimerspec,
> +                struct __kernel_old_itimerval],,,[#include <sys/socket.h>])

@Hui following is unrelated to your patchset (we can sort it later, it should
not block your effort).

@Cyril the original code prior this patchset in 203ee275c ("Fix struct
__kernel_old_timeval redefinition on 64bit sparc") did not include
<linux/time_types.h> for some reason IMHO fallbacks were always used.
I wonder why and whether we still don't want to use <linux/time_types.h>.

Then Fabrice's fix in 12986b755 ("include/tst_timer.h: avoid redefinition of
kernel structures") add autotools check just for uncommon toolchain (sh4 from
Texas Instruments). It's somehow hidden (due missing comment it looks like we
mostly get the definitions from header, but obviously not when we include
<sys/socket.h>.

>  AC_CHECK_TYPES([struct futex_waitv],,,[#include <linux/futex.h>])
>  AC_CHECK_TYPES([struct mount_attr],,,[
> diff --git a/include/tst_timer.h b/include/tst_timer.h
> index 703f03294eae..6fb9400206b8 100644
> --- a/include/tst_timer.h
> +++ b/include/tst_timer.h
> @@ -135,6 +135,13 @@ struct __kernel_itimerspec {
>  	struct __kernel_timespec it_value;       /* timer expiration */
>  };
>  #endif
> +
> +#ifndef HAVE_STRUCT___KERNEL_OLD_ITIMERVAL
> +struct __kernel_old_itimerval {
> +	struct __kernel_old_timeval it_interval;	/* timer interval */
> +	struct __kernel_old_timeval it_value;		/* current value */
> +};
> +#endif
>  #endif

>  enum tst_ts_type {
> @@ -370,6 +377,11 @@ static inline int sys_timerfd_settime64(int fd, int flags, void *its,
>  	return tst_syscall(__NR_timerfd_settime64, fd, flags, its, old_its);
>  }

> +static inline int sys_setitimer(int which, void *new_value, void *old_value)
> +{
> +	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> +}

+1 adding function to the common place.

IMHO we slightly prefer to add C functions to C file (e.g. lib/tst_timer.c,
there are other functions) + adding signature to tst_timer.h.

With that:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> +
>  /*
>   * Returns tst_ts seconds.
>   */
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
> index d12abe904f1c..94ee51c6a667 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -20,9 +20,10 @@
>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
>  #include "tst_safe_clocks.h"
> +#include "tst_timer.h"

>  static struct timeval tv;
> -static struct itimerval *value, *ovalue;
> +static struct __kernel_old_itimerval *value, *ovalue;
>  static volatile unsigned long sigcnt;
>  static long time_step;
>  static long time_sec;
> @@ -38,11 +39,6 @@ static struct tcase {
>  	{ITIMER_PROF,    "ITIMER_PROF",    SIGPROF},
>  };

> -static int sys_setitimer(int which, void *new_value, void *old_value)
> -{
> -	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> -}
> -
>  static void sig_routine(int signo LTP_ATTRIBUTE_UNUSED)
>  {
>  	sigcnt++;
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer02.c b/testcases/kernel/syscalls/setitimer/setitimer02.c
> index b012d71fa7bd..c545f6288bbb 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer02.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer02.c
> @@ -19,13 +19,9 @@
>  #include <stdlib.h>
>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
> +#include "tst_timer.h"

> -static struct itimerval *value, *ovalue;
> -
> -static int sys_setitimer(int which, void *new_value, void *old_value)
> -{
> -	return tst_syscall(__NR_setitimer, which, new_value, old_value);
> -}
> +static struct __kernel_old_itimerval *value, *ovalue;

>  static void verify_setitimer(unsigned int i)
>  {
> @@ -55,8 +51,8 @@ static struct tst_test test = {
>  	.test = verify_setitimer,
>  	.setup = setup,
>  	.bufs = (struct tst_buffers[]) {
> -		{&value,  .size = sizeof(struct itimerval)},
> -		{&ovalue, .size = sizeof(struct itimerval)},
> +		{&value,  .size = sizeof(struct __kernel_old_itimerval)},
> +		{&ovalue, .size = sizeof(struct __kernel_old_itimerval)},
>  		{}
>  	}
>  };

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