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

[Petr Vorel <pvorel@suse.cz>]

Hi all,

thanks, patchset merged.

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

Right. Maybe this could then replace <sys/socket.h>, but I would have to verify
it running also Buildroot CI.

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

Yes, uncommon toolchain.

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

Thanks for investigation.

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

+1. Adding .test_variants with {g,s}etitimer() for {g,s}etitimer0[12].c should
be trivial. Or did you mean to have test variants also for other functions in
include/tst_timer.h (e.g. timerfd_{g,s}ettime()) ?

BTW we already test {g,s}etitimer() libc wrapper in getitimer01.c, therefore we
already have libc wrapper at least somehow covered (the other two test raw
syscall).

Once we agree what to do I'll either post a patch or add a ticket for it (or
feel free to add a ticket yourself).

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

Ah, I was wrong. There are also many other single line functions in
include/tst_timer.h.

Kind regards,
Petr

> So for the patch as it is:

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

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