| Message ID | 931bddab3d92f73f07f32dd7e1770078fdc07e0e.1589877853.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | syscalls: Remove incorrect usage of libc structures | expand |
On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Use gettimeofday() instead of calling it with tst_syscall(). > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- I think the change makes it work reliably, but it does change what you are testing for: instead of testing the low-level system call interface, this will now test the libc interface, which is implemented on top of the vdso or clock_gettime(). I think all variants (vdso, syscall(__NR_gettimeofday), libc gettimeofday, emulation with clock_gettime syscall/vdso/libc) need to be tested. It's possible they all are, but that should be clarified in the changelog text. Arnd
On 19-05-20, 11:20, Arnd Bergmann wrote: > On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > Use gettimeofday() instead of calling it with tst_syscall(). > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > I think the change makes it work reliably, but it does change what you > are testing for: instead of testing the low-level system call interface, > this will now test the libc interface, which is implemented on top > of the vdso or clock_gettime(). Actually the testcase was for settimeofday() and we were unnecessarily calling gettimeofday with tst_syscall(). And so the testcase should remain unaffected that way. Had this been a testcase for gettimeofday(), I would have agreed with you. > I think all variants (vdso, syscall(__NR_gettimeofday), libc > gettimeofday, emulation with clock_gettime syscall/vdso/libc) > need to be tested. It's possible they all are, but that should > be clarified in the changelog text. They aren't tested that way.
On Tue, May 19, 2020 at 11:25 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-05-20, 11:20, Arnd Bergmann wrote: > > On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > Use gettimeofday() instead of calling it with tst_syscall(). > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > > I think the change makes it work reliably, but it does change what you > > are testing for: instead of testing the low-level system call interface, > > this will now test the libc interface, which is implemented on top > > of the vdso or clock_gettime(). > > Actually the testcase was for settimeofday() and we were unnecessarily > calling gettimeofday with tst_syscall(). And so the testcase should > remain unaffected that way. Had this been a testcase for > gettimeofday(), I would have agreed with you. Ok, makes sense. Just mention in the changelog text that this fixes running against an updated libc with 64-bit time_t in that case. Arnd
Hi! You can set the .restore_wallclock flag in the tst_test structure for these two tests and remove the setup() and cleanup() as well. Also in the settimeofday02 I would be inclined to just remove the clock restoration code, since there is no way the tim will be changed unless the kernel is buggy.
On Tue, May 19, 2020 at 2:15 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > You can set the .restore_wallclock flag in the tst_test structure for > these two tests and remove the setup() and cleanup() as well. > > Also in the settimeofday02 I would be inclined to just remove the clock > restoration code, since there is no way the tim will be changed unless > the kernel is buggy. Ah, I did not realize that LTP actually does try to set the clock and then set it back. If it does that, it may be very interesting to test the behavior across the y2038 overflow, e.g. set the time ot just after the 2038-01-19 expiration and ensure the kernel still behaves as expected, or set the timer to just before the overflow and set a timer just after, and see that the timer triggers. Arnd
On 19-05-20, 14:16, Cyril Hrubis wrote: > Hi! > You can set the .restore_wallclock flag in the tst_test structure for > these two tests and remove the setup() and cleanup() as well. Done. > Also in the settimeofday02 I would be inclined to just remove the clock > restoration code, since there is no way the tim will be changed unless > the kernel is buggy. If I understand it correctly, you now suggested to get rid of the .restore_wallclock() flag from 02.c ? I think it is better to keep it as the kernel can be buggy and we must get back to a working state in such a case. So, I didn't remove it :)
Hi! > > Also in the settimeofday02 I would be inclined to just remove the clock > > restoration code, since there is no way the tim will be changed unless > > the kernel is buggy. > > If I understand it correctly, you now suggested to get rid of the > .restore_wallclock() flag from 02.c ? I think it is better to keep it as the > kernel can be buggy and we must get back to a working state in such a case. So, > I didn't remove it :) If kernel is buggy, all bets are off and we do not guarantee to leave the system in an usable state.
diff --git a/testcases/kernel/syscalls/settimeofday/settimeofday01.c b/testcases/kernel/syscalls/settimeofday/settimeofday01.c index 368fdebc0c8e..c599a820fc97 100644 --- a/testcases/kernel/syscalls/settimeofday/settimeofday01.c +++ b/testcases/kernel/syscalls/settimeofday/settimeofday01.c @@ -23,7 +23,7 @@ static void verify_settimeofday(void) suseconds_t delta; struct timeval tv1, tv2; - if (tst_syscall(__NR_gettimeofday, &tv1, NULL) == -1) + if (gettimeofday(&tv1, NULL) == -1) tst_brk(TBROK | TERRNO, "gettimeofday(&tv1, NULL) failed"); tv1.tv_sec += VAL_SEC; @@ -37,7 +37,7 @@ static void verify_settimeofday(void) return; } - if (tst_syscall(__NR_gettimeofday, &tv2, NULL) == -1) + if (gettimeofday(&tv2, NULL) == -1) tst_brk(TBROK | TERRNO, "gettimeofday(&tv2, NULL) failed"); if (tv2.tv_sec > tv1.tv_sec) { @@ -58,7 +58,7 @@ static void verify_settimeofday(void) static void setup(void) { - if (tst_syscall(__NR_gettimeofday, &tv_saved, NULL) == -1) + if (gettimeofday(&tv_saved, NULL) == -1) tst_brk(TBROK | TERRNO, "gettimeofday(&tv_saved, NULL) failed"); } diff --git a/testcases/kernel/syscalls/settimeofday/settimeofday02.c b/testcases/kernel/syscalls/settimeofday/settimeofday02.c index 485a26b1d9c5..0d6862eb33b1 100644 --- a/testcases/kernel/syscalls/settimeofday/settimeofday02.c +++ b/testcases/kernel/syscalls/settimeofday/settimeofday02.c @@ -46,7 +46,7 @@ static void verify_settimeofday(unsigned int n) static void setup(void) { - if (tst_syscall(__NR_gettimeofday, &tv_saved, NULL) == -1) + if (gettimeofday(&tv_saved, NULL) == -1) tst_brk(TBROK | TERRNO, "gettimeofday(&tv_saved, NULL) failed"); }
Use gettimeofday() instead of calling it with tst_syscall(). Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- testcases/kernel/syscalls/settimeofday/settimeofday01.c | 6 +++--- testcases/kernel/syscalls/settimeofday/settimeofday02.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)