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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> @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>.

I suppose that this is broken on some old distro, try to run that
through CI and if that passes we can do so.

> 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>.

I guess that it depends on architecture/libc/kernel headers and it's a
big mess...

> >  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


I've been staring at the kernel and libc code for a while and it seems
that there is not itimerval64 syscall and interval timers are limited to
32bit on 32bit architectures. In reality I suppose that it does not
matter since nobody is going to use intervals that actually need 64bit
amount of seconds anyways.

So libc takes 64bit itimer, converts it to 32bit and kernel does
the oposite conversion.

Also we should really add tests for the libc wrapper as well, since that
is actually more likely to get broken by the double conversion on 32bit
arch, but that should be done in an subsequent patches.

> >  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);
> > +}
> C
> +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.

I would say that there is no point to do that for a single line fuctions
like this and actually I guess that this would break the line numbers
and filenames for the tst_sycall() so it's better this way.

So for the patch as it is:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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