| Message ID | 3f3b7d669d47ae701385b43deb8280a353dd231e.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: > @@ -472,7 +471,7 @@ static uint64_t get_current_time(void) > { > struct timespec ts; > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > + clock_gettime(CLOCK_MONOTONIC, &ts); Again, you are changing from the low-level syscall interface that is not present on new 32-bit architectures which only have clock_gettime64 to a high-level interface. This change correctly changes it to pass matching timespec variant, but the changelog text does not mention that. Arnd
On 19-05-20, 11:22, Arnd Bergmann wrote: > On Tue, May 19, 2020 at 10:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > @@ -472,7 +471,7 @@ static uint64_t get_current_time(void) > > { > > struct timespec ts; > > > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > Again, you are changing from the low-level syscall interface that is not > present on new 32-bit architectures which only have clock_gettime64 > to a high-level interface. > > This change correctly changes it to pass matching timespec variant, but > the changelog text does not mention that. Right, I can actually mention why this change was necessary.
Hi! > These syscall are old enough and must have support in libc for everyone. > Don't use tst_syscall() for them unnecessarily. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > lib/parse_opts.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/lib/parse_opts.c b/lib/parse_opts.c > index a9d50589a3f9..b3ab69c0a539 100644 > --- a/lib/parse_opts.c > +++ b/lib/parse_opts.c > @@ -45,7 +45,6 @@ > #include "test.h" > #include "ltp_priv.h" > #include "usctest.h" > -#include "tst_clocks.h" > > #ifndef UNIT_TEST > #define UNIT_TEST 0 > @@ -472,7 +471,7 @@ static uint64_t get_current_time(void) > { > struct timespec ts; > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > + clock_gettime(CLOCK_MONOTONIC, &ts); I guess that this will reintroduce LTP compilation failures on older glibc, which was the primary reason we used the tst_clock_gettime() instead of clock_gettime().
On Tue, May 19, 2020 at 2:23 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > These syscall are old enough and must have support in libc for everyone. > > Don't use tst_syscall() for them unnecessarily. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > lib/parse_opts.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/lib/parse_opts.c b/lib/parse_opts.c > > index a9d50589a3f9..b3ab69c0a539 100644 > > --- a/lib/parse_opts.c > > +++ b/lib/parse_opts.c > > @@ -45,7 +45,6 @@ > > #include "test.h" > > #include "ltp_priv.h" > > #include "usctest.h" > > -#include "tst_clocks.h" > > > > #ifndef UNIT_TEST > > #define UNIT_TEST 0 > > @@ -472,7 +471,7 @@ static uint64_t get_current_time(void) > > { > > struct timespec ts; > > > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > I guess that this will reintroduce LTP compilation failures on older > glibc, which was the primary reason we used the tst_clock_gettime() > instead of clock_gettime(). I see that clock_gettime was first added in glibc-2.1.3 back in 1999. Can that actually run LTP any more? If it can and this is considered important, I fear the tst_clock_gettime() call needs to be extended to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls, whichever is the first to work, and convert the formats from the native kernel format to the glibc format. Arnd
Hi, > > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > I guess that this will reintroduce LTP compilation failures on older > > glibc, which was the primary reason we used the tst_clock_gettime() > > instead of clock_gettime(). > I see that clock_gettime was first added in glibc-2.1.3 back in 1999. > Can that actually run LTP any more? If it can and this is considered > important, I fear the tst_clock_gettime() call needs to be extended > to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls, > whichever is the first to work, and convert the formats from the > native kernel format to the glibc format. IMHO the older system we still test in Travis (but going to remove it soon) is CentOS 6 (kernel 3.10, glibc 2.12, gcc 4.4.7). I suspect that it was needed this system (e.g. system with old glibc and gcc; gcc required some fixes which bothered me, but old glibc actually caught some bugs in fallback which we wouldn't otherwise find). Or am I wrong? We agreed (few LTP maintainers), that, at least for SUSE and Red Hat is ok to drop support for distros 10+ years, because these systems are tested with some older LTP release anyway. > Arnd Kind regards, Petr
Hi! > > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > > > I guess that this will reintroduce LTP compilation failures on older > > glibc, which was the primary reason we used the tst_clock_gettime() > > instead of clock_gettime(). > > I see that clock_gettime was first added in glibc-2.1.3 back in 1999. > Can that actually run LTP any more? If it can and this is considered > important, I fear the tst_clock_gettime() call needs to be extended > to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls, > whichever is the first to work, and convert the formats from the > native kernel format to the glibc format. I guess that at the current time we do support distros that are at max 10 years old, mostly because enterprise support cycles are about 10 years in lenght. The issue here is that glibc needed -lrt passed to linker couple of years ago and we wanted to avoid the need of linking everything with -lrt, as calling the raw syscall was just easier fix.
On 19-05-20, 15:45, Cyril Hrubis wrote: > Hi! > > > > - tst_clock_gettime(CLOCK_MONOTONIC, &ts); > > > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > > > > > I guess that this will reintroduce LTP compilation failures on older > > > glibc, which was the primary reason we used the tst_clock_gettime() > > > instead of clock_gettime(). > > > > I see that clock_gettime was first added in glibc-2.1.3 back in 1999. > > Can that actually run LTP any more? If it can and this is considered > > important, I fear the tst_clock_gettime() call needs to be extended > > to call the clock_gettime()/clock_gettime64()/gettimeofday() syscalls, > > whichever is the first to work, and convert the formats from the > > native kernel format to the glibc format. > > I guess that at the current time we do support distros that are at max > 10 years old, mostly because enterprise support cycles are about 10 > years in lenght. > > The issue here is that glibc needed -lrt passed to linker couple of > years ago and we wanted to avoid the need of linking everything with > -lrt, as calling the raw syscall was just easier fix. To conclude the discussion, is this patch okay or not ? The reason why I am asking this is because this file still uses the old test framework and so can't include tst_timer.h, and so doesn't have access to struct __kernel_old_timespec. What do you suggest I do here ?
Hi! > > I guess that at the current time we do support distros that are at max > > 10 years old, mostly because enterprise support cycles are about 10 > > years in lenght. > > > > The issue here is that glibc needed -lrt passed to linker couple of > > years ago and we wanted to avoid the need of linking everything with > > -lrt, as calling the raw syscall was just easier fix. > > To conclude the discussion, is this patch okay or not ? The reason why I am > asking this is because this file still uses the old test framework and so can't > include tst_timer.h, and so doesn't have access to struct __kernel_old_timespec. > > What do you suggest I do here ? Well if it was only about __kernel_old_timespec we could easily just declare it locally in the file and be done with it. But I guess that newer 32bit architectures will have only the 64 bit syscall present, I think that somebody pointed out that this is the case for 32bit RiscV. If that's true we will have to have a fallback for that case as well.
On Thu, May 21, 2020 at 4:20 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > > I guess that at the current time we do support distros that are at max > > > 10 years old, mostly because enterprise support cycles are about 10 > > > years in lenght. > > > > > > The issue here is that glibc needed -lrt passed to linker couple of > > > years ago and we wanted to avoid the need of linking everything with > > > -lrt, as calling the raw syscall was just easier fix. > > > > To conclude the discussion, is this patch okay or not ? The reason why I am > > asking this is because this file still uses the old test framework and so can't > > include tst_timer.h, and so doesn't have access to struct __kernel_old_timespec. > > > > What do you suggest I do here ? > > Well if it was only about __kernel_old_timespec we could easily just > declare it locally in the file and be done with it. > > But I guess that newer 32bit architectures will have only the 64 bit > syscall present, I think that somebody pointed out that this is the case > for 32bit RiscV. If that's true we will have to have a fallback for that > case as well. Yes, that is correct. Also, the 32-bit time_t syscalls can be disabled on newer kernels when you have a libc implementation that uses the 64-bit calls exclusively. I guess the fallback to to libc gettimeofday() isn't really needed unless it needs to run on ancient kernels. Arnd
diff --git a/lib/parse_opts.c b/lib/parse_opts.c index a9d50589a3f9..b3ab69c0a539 100644 --- a/lib/parse_opts.c +++ b/lib/parse_opts.c @@ -45,7 +45,6 @@ #include "test.h" #include "ltp_priv.h" #include "usctest.h" -#include "tst_clocks.h" #ifndef UNIT_TEST #define UNIT_TEST 0 @@ -472,7 +471,7 @@ static uint64_t get_current_time(void) { struct timespec ts; - tst_clock_gettime(CLOCK_MONOTONIC, &ts); + clock_gettime(CLOCK_MONOTONIC, &ts); return (((uint64_t) ts.tv_sec) * USECS_PER_SEC) + ts.tv_nsec / 1000; }
These syscall are old enough and must have support in libc for everyone. Don't use tst_syscall() for them unnecessarily. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- lib/parse_opts.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)