diff mbox series

Don't test nanoseconds for non-LFS interface in io/tst-stat.c

Message ID 20210316135612.1400708-1-stli@linux.ibm.com
State New
Headers show
Series Don't test nanoseconds for non-LFS interface in io/tst-stat.c | expand

Commit Message

Stefan Liebler March 16, 2021, 1:56 p.m. UTC 
Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64)
are comparing the nanosecond fields with the statx result.  Unfortunately
e.g. on s390(31bit) those fields are always zero if old KABI with non-LFS
support is used.  With _FILE_OFFSET_BITS=64 stat is using statx internally.

As suggested by Adhemerval this patch disables the nanosecond check for
non-LFS interface.
---
 io/tst-stat.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Adhemerval Zanella Netto March 16, 2021, 2:24 p.m. UTC | #1 
On 16/03/2021 10:56, Stefan Liebler wrote:
> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64)
> are comparing the nanosecond fields with the statx result.  Unfortunately
> e.g. on s390(31bit) those fields are always zero if old KABI with non-LFS
> support is used.  With _FILE_OFFSET_BITS=64 stat is using statx internally.

This might also happens for LFS interface if statx is not supported by the
kernel, since the LFS call will fall back to the use the stat syscall that
has this issue.

Maybe it would be better to make it an internal tests and add a flag
somewhere to just disable it for s390-32. 

> 
> As suggested by Adhemerval this patch disables the nanosecond check for
> non-LFS interface.
> ---
>  io/tst-stat.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/io/tst-stat.c b/io/tst-stat.c
> index 445ac4176c..550128a2a5 100644
> --- a/io/tst-stat.c
> +++ b/io/tst-stat.c
> @@ -26,6 +26,20 @@
>  #include <sys/stat.h>
>  #include <sys/sysmacros.h>
>  #include <unistd.h>
> +#include <kernel_stat.h>
> +
> +#if defined _FILE_OFFSET_BITS && _FILE_OFFSET_BITS == 64
> +/* Test nanoseconds if LFS interface is requested.  */
> +# define TEST_NANOSECONDS 1
> +#elif !XSTAT_IS_XSTAT64
> +/* LFS interface is not requested and we have no LFS support.  E.g. s390(31bit)
> +   is using old KABI with old non-LFS support and the nanoseconds fields are
> +   always zero.  */
> +# define TEST_NANOSECONDS 0
> +#else
> +/* LFS interface is not requested, but we have LFS support.  */
> +# define TEST_NANOSECONDS 1
> +#endif
>  
>  static void
>  stat_check (int fd, const char *path, struct stat *st)
> @@ -91,9 +105,11 @@ do_test (void)
>        TEST_COMPARE (stx.stx_blocks, st.st_blocks);
>  
>        TEST_COMPARE (stx.stx_ctime.tv_sec, st.st_ctim.tv_sec);
> -      TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec);
>        TEST_COMPARE (stx.stx_mtime.tv_sec, st.st_mtim.tv_sec);
> +#if TEST_NANOSECONDS
> +      TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec);
>        TEST_COMPARE (stx.stx_mtime.tv_nsec, st.st_mtim.tv_nsec);
> +#endif
>      }
>  
>    return 0;
>
Stefan Liebler March 16, 2021, 4:30 p.m. UTC | #2 
On 16/03/2021 15:24, Adhemerval Zanella wrote:
> 
> 
> On 16/03/2021 10:56, Stefan Liebler wrote:
>> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64)
>> are comparing the nanosecond fields with the statx result.  Unfortunately
>> e.g. on s390(31bit) those fields are always zero if old KABI with non-LFS
>> support is used.  With _FILE_OFFSET_BITS=64 stat is using statx internally.
> 
> This might also happens for LFS interface if statx is not supported by the
> kernel, since the LFS call will fall back to the use the stat syscall that
> has this issue.
> 
> Maybe it would be better to make it an internal tests and add a flag
> somewhere to just disable it for s390-32. 
> 
gcc is setting the macros __s390x__ and __s390__ on s390x(64bit), but
only __s390__ on s390(31bit). Thus I can detect s390(31bit) at
compile-time via "#if" and disable the nanoseconds checks on s390(31bit)
at all and run it on all other cases. Then I don't need to make the test
an internal test and don't need special flags. If this is okay for you,
I will prepare a further patch (also with a different subject).

Thanks,
Stefan
Adhemerval Zanella Netto March 16, 2021, 7:59 p.m. UTC | #3 
On 16/03/2021 13:30, Stefan Liebler wrote:
> On 16/03/2021 15:24, Adhemerval Zanella wrote:
>>
>>
>> On 16/03/2021 10:56, Stefan Liebler wrote:
>>> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64)
>>> are comparing the nanosecond fields with the statx result.  Unfortunately
>>> e.g. on s390(31bit) those fields are always zero if old KABI with non-LFS
>>> support is used.  With _FILE_OFFSET_BITS=64 stat is using statx internally.
>>
>> This might also happens for LFS interface if statx is not supported by the
>> kernel, since the LFS call will fall back to the use the stat syscall that
>> has this issue.
>>
>> Maybe it would be better to make it an internal tests and add a flag
>> somewhere to just disable it for s390-32. 
>>
> gcc is setting the macros __s390x__ and __s390__ on s390x(64bit), but
> only __s390__ on s390(31bit). Thus I can detect s390(31bit) at
> compile-time via "#if" and disable the nanoseconds checks on s390(31bit)
> at all and run it on all other cases. Then I don't need to make the test
> an internal test and don't need special flags. If this is okay for you,
> I will prepare a further patch (also with a different subject).

I think it would better to add such logic on libsupport instead directly
on the test itself, similar to what support_path_support_time64 does. 
Maybe something like:

  bool
  support_stat_nanoseconds (void)
  {
    /* s390 stat64 compat symbol does not support nanoseconds resolution
       and it used on non-LFS [f,l]stat[at] implementations.  */
  #if defined __linux__ && !defined __s390x__ && defined __s390__
    return false;
  #else
    return true;
  }

Another possibility is if you want to fix it for s390 is to call
statx on Linux non-LFS fstatat call (sysdeps/unix/sysv/linux/fstatat.c)
which should fix stat, lstat, and fstat as well.  You will need to
add a stat to statx conversion at sysdeps/unix/sysv/linux/statx_cp.c
which would need handle EOVERFLOW on st_ino, st_size, and st_blocks.
Stefan Liebler March 17, 2021, 1:04 p.m. UTC | #4 
On 16/03/2021 20:59, Adhemerval Zanella wrote:
> 
> 
> On 16/03/2021 13:30, Stefan Liebler wrote:
>> On 16/03/2021 15:24, Adhemerval Zanella wrote:
>>>
>>>
>>> On 16/03/2021 10:56, Stefan Liebler wrote:
>>>> Both new tests io/tst-stat and io/tst-stat-lfs (_FILE_OFFSET_BITS=64)
>>>> are comparing the nanosecond fields with the statx result.  Unfortunately
>>>> e.g. on s390(31bit) those fields are always zero if old KABI with non-LFS
>>>> support is used.  With _FILE_OFFSET_BITS=64 stat is using statx internally.
>>>
>>> This might also happens for LFS interface if statx is not supported by the
>>> kernel, since the LFS call will fall back to the use the stat syscall that
>>> has this issue.
>>>
>>> Maybe it would be better to make it an internal tests and add a flag
>>> somewhere to just disable it for s390-32. 
>>>
>> gcc is setting the macros __s390x__ and __s390__ on s390x(64bit), but
>> only __s390__ on s390(31bit). Thus I can detect s390(31bit) at
>> compile-time via "#if" and disable the nanoseconds checks on s390(31bit)
>> at all and run it on all other cases. Then I don't need to make the test
>> an internal test and don't need special flags. If this is okay for you,
>> I will prepare a further patch (also with a different subject).
> 
> I think it would better to add such logic on libsupport instead directly
> on the test itself, similar to what support_path_support_time64 does. 
> Maybe something like:
> 
>   bool
>   support_stat_nanoseconds (void)
>   {
>     /* s390 stat64 compat symbol does not support nanoseconds resolution
>        and it used on non-LFS [f,l]stat[at] implementations.  */
>   #if defined __linux__ && !defined __s390x__ && defined __s390__
>     return false;
>   #else
>     return true;
>   }
> 
> Another possibility is if you want to fix it for s390 is to call
> statx on Linux non-LFS fstatat call (sysdeps/unix/sysv/linux/fstatat.c)
> which should fix stat, lstat, and fstat as well.  You will need to
> add a stat to statx conversion at sysdeps/unix/sysv/linux/statx_cp.c
> which would need handle EOVERFLOW on st_ino, st_size, and st_blocks.
> 

I'm fine with just disabling the nanoseconds test on s390(31bit). I've
also talked to the kernel guys. They won't fix the compat layer in this
case.

Please have a look at the new patch
"[PATCH] S390: Don't test nanoseconds in io/tst-stat.c"
https://sourceware.org/pipermail/libc-alpha/2021-March/124063.html

Thanks,
Stefan
diff mbox series

Patch

diff --git a/io/tst-stat.c b/io/tst-stat.c
index 445ac4176c..550128a2a5 100644
--- a/io/tst-stat.c
+++ b/io/tst-stat.c
@@ -26,6 +26,20 @@ 
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
 #include <unistd.h>
+#include <kernel_stat.h>
+
+#if defined _FILE_OFFSET_BITS && _FILE_OFFSET_BITS == 64
+/* Test nanoseconds if LFS interface is requested.  */
+# define TEST_NANOSECONDS 1
+#elif !XSTAT_IS_XSTAT64
+/* LFS interface is not requested and we have no LFS support.  E.g. s390(31bit)
+   is using old KABI with old non-LFS support and the nanoseconds fields are
+   always zero.  */
+# define TEST_NANOSECONDS 0
+#else
+/* LFS interface is not requested, but we have LFS support.  */
+# define TEST_NANOSECONDS 1
+#endif
 
 static void
 stat_check (int fd, const char *path, struct stat *st)
@@ -91,9 +105,11 @@  do_test (void)
       TEST_COMPARE (stx.stx_blocks, st.st_blocks);
 
       TEST_COMPARE (stx.stx_ctime.tv_sec, st.st_ctim.tv_sec);
-      TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec);
       TEST_COMPARE (stx.stx_mtime.tv_sec, st.st_mtim.tv_sec);
+#if TEST_NANOSECONDS
+      TEST_COMPARE (stx.stx_ctime.tv_nsec, st.st_ctim.tv_nsec);
       TEST_COMPARE (stx.stx_mtime.tv_nsec, st.st_mtim.tv_nsec);
+#endif
     }
 
   return 0;