tst: Add test for utime

  This patch provides test for utime. It uses xfstat() syscall to read
access and modification times to compare with ones written by utime.

Moreover, access and modification times beyond the Y2038 threshold
date (i.e. 32 bit time_t overflow) are also checked.
---
 sysdeps/unix/sysv/linux/Makefile    |  2 +-
 sysdeps/unix/sysv/linux/tst-utime.c | 83 +++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-utime.c
  

On 24/02/2021 21:35, Lukasz Majewski wrote:
> This patch provides test for utime. It uses xfstat() syscall to read
> access and modification times to compare with ones written by utime.

Same as for futimens tests, it does not need to specific the internal
function used.

> 
> Moreover, access and modification times beyond the Y2038 threshold
> date (i.e. 32 bit time_t overflow) are also checked.

LGTM with the commit message change the small nit below.

I am not sure if it is worth to silent not run the some internal tests
for 32 bit time, but we can improve it once we get 64 bit time_t
support.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/unix/sysv/linux/Makefile    |  2 +-
>  sysdeps/unix/sysv/linux/tst-utime.c | 83 +++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-utime.c
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 4e043c78ab..e68c259fa0 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -107,7 +107,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
>  	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
>  	 tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \
>  	 tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \
> -	 tst-timerfd tst-ppoll tst-futimens
> +	 tst-timerfd tst-ppoll tst-futimens tst-utime
>  tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc
>  
>  CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables

Ok.

> diff --git a/sysdeps/unix/sysv/linux/tst-utime.c b/sysdeps/unix/sysv/linux/tst-utime.c
> new file mode 100644
> index 0000000000..fe1869702c
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-utime.c
> @@ -0,0 +1,83 @@
> +/* Test for utime

Missing period.

> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <time.h>
> +#include <stdlib.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <utime.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +#include <support/temp_file.h>
> +
> +static int temp_fd = -1;
> +char *testfile;
> +
> +/* struct utimbuf with Y2038 threshold minus 2 and 1 seconds.  */
> +const struct utimbuf t1 = { 0x7FFFFFFE, 0x7FFFFFFF };
> +
> +/* struct utimbuf with Y2038 threshold plus 1 and 2 seconds.  */
> +const struct utimbuf t2 = { 0x80000001ULL, 0x80000002ULL };
> +
> +/* struct utimbuf around Y2038 threshold.  */
> +const struct utimbuf t3 = { 0x7FFFFFFE, 0x80000002ULL };

Use static on these.

> +
> +#define PREPARE do_prepare
> +static void
> +do_prepare (int argc, char *argv[])
> +{
> +  temp_fd = create_temp_file ("utime", &testfile);
> +  TEST_VERIFY_EXIT (temp_fd > 0);
> +}
> +
> +static int
> +test_utime_helper (const struct utimbuf *ut)
> +{
> +  struct stat64 st;
> +  int result;
> +  time_t t;
> +
> +  /* Check if we run on port with 32 bit time_t size */
> +  if (__builtin_add_overflow (ut->actime, 0, &t))
> +    return 0;

Maybe add a warning that this specific test is not executed due
32 bit time_t? Same for futimens.

> +
> +  result = utime(testfile, ut);
> +  TEST_VERIFY_EXIT (result == 0);
> +
> +  xfstat (temp_fd, &st);
> +
> +  /* Check if seconds for actime match */
> +  TEST_COMPARE (st.st_atime, ut->actime);
> +
> +  /* Check if seconds for modtime match */
> +  TEST_COMPARE (st.st_mtime, ut->modtime);
> +
> +  return 0;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  test_utime_helper (&t1);
> +  test_utime_helper (&t2);
> +  test_utime_helper (&t3);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 

Ok.
  

Hi Adhemerval,

> On 24/02/2021 21:35, Lukasz Majewski wrote:
> > This patch provides test for utime. It uses xfstat() syscall to read
> > access and modification times to compare with ones written by
> > utime.  
> 
> Same as for futimens tests, it does not need to specific the internal
> function used.
> 
> > 
> > Moreover, access and modification times beyond the Y2038 threshold
> > date (i.e. 32 bit time_t overflow) are also checked.  
> 
> LGTM with the commit message change the small nit below.
> 
> I am not sure if it is worth to silent not run the some internal tests
> for 32 bit time, but we can improve it once we get 64 bit time_t
> support.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> > ---
> >  sysdeps/unix/sysv/linux/Makefile    |  2 +-
> >  sysdeps/unix/sysv/linux/tst-utime.c | 83
> > +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1
> > deletion(-) create mode 100644 sysdeps/unix/sysv/linux/tst-utime.c
> > 
> > diff --git a/sysdeps/unix/sysv/linux/Makefile
> > b/sysdeps/unix/sysv/linux/Makefile index 4e043c78ab..e68c259fa0
> > 100644 --- a/sysdeps/unix/sysv/linux/Makefile
> > +++ b/sysdeps/unix/sysv/linux/Makefile
> > @@ -107,7 +107,7 @@ tests += tst-clone tst-clone2 tst-clone3
> > tst-fanotify tst-personality \ test-errno-linux tst-memfd_create
> > tst-mlock2 tst-pkey \ tst-rlimit-infinity tst-ofdlocks tst-gettid
> > tst-gettid-kill \ tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux
> > tst-sysvshm-linux \
> > -	 tst-timerfd tst-ppoll tst-futimens
> > +	 tst-timerfd tst-ppoll tst-futimens tst-utime
> >  tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc
> >  
> >  CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables  
> 
> Ok.
> 
> > diff --git a/sysdeps/unix/sysv/linux/tst-utime.c
> > b/sysdeps/unix/sysv/linux/tst-utime.c new file mode 100644
> > index 0000000000..fe1869702c
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/tst-utime.c
> > @@ -0,0 +1,83 @@
> > +/* Test for utime  
> 
> Missing period.
> 
> > +   Copyright (C) 2021 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it
> > and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later
> > version. +
> > +   The GNU C Library is distributed in the hope that it will be
> > useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <time.h>
> > +#include <stdlib.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <utime.h>
> > +#include <support/check.h>
> > +#include <support/xunistd.h>
> > +#include <support/temp_file.h>
> > +
> > +static int temp_fd = -1;
> > +char *testfile;
> > +
> > +/* struct utimbuf with Y2038 threshold minus 2 and 1 seconds.  */
> > +const struct utimbuf t1 = { 0x7FFFFFFE, 0x7FFFFFFF };
> > +
> > +/* struct utimbuf with Y2038 threshold plus 1 and 2 seconds.  */
> > +const struct utimbuf t2 = { 0x80000001ULL, 0x80000002ULL };
> > +
> > +/* struct utimbuf around Y2038 threshold.  */
> > +const struct utimbuf t3 = { 0x7FFFFFFE, 0x80000002ULL };  
> 
> Use static on these.
> 
> > +
> > +#define PREPARE do_prepare
> > +static void
> > +do_prepare (int argc, char *argv[])
> > +{
> > +  temp_fd = create_temp_file ("utime", &testfile);
> > +  TEST_VERIFY_EXIT (temp_fd > 0);
> > +}
> > +
> > +static int
> > +test_utime_helper (const struct utimbuf *ut)
> > +{
> > +  struct stat64 st;
> > +  int result;
> > +  time_t t;
> > +
> > +  /* Check if we run on port with 32 bit time_t size */
> > +  if (__builtin_add_overflow (ut->actime, 0, &t))
> > +    return 0;  
> 
> Maybe add a warning that this specific test is not executed due
> 32 bit time_t? Same for futimens.
> 

Would it be correct to use here for example the FAIL_RET(), as the
return value is not checked in do_test() ?

Or will it be just enough to use plain printf() ?

> > +
> > +  result = utime(testfile, ut);
> > +  TEST_VERIFY_EXIT (result == 0);
> > +
> > +  xfstat (temp_fd, &st);
> > +
> > +  /* Check if seconds for actime match */
> > +  TEST_COMPARE (st.st_atime, ut->actime);
> > +
> > +  /* Check if seconds for modtime match */
> > +  TEST_COMPARE (st.st_mtime, ut->modtime);
> > +
> > +  return 0;
> > +}
> > +
> > +static int
> > +do_test (void)
> > +{
> > +  test_utime_helper (&t1);
> > +  test_utime_helper (&t2);
> > +  test_utime_helper (&t3);
> > +
> > +  return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
> >   
> 
> Ok.




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On 27/02/2021 18:38, Lukasz Majewski wrote:
>>
>> Maybe add a warning that this specific test is not executed due
>> 32 bit time_t? Same for futimens.
>>
> 
> Would it be correct to use here for example the FAIL_RET(), as the
> return value is not checked in do_test() ?
> 
> Or will it be just enough to use plain printf() ?

We have the support_record_failure (), but I don't think this should
be considered an error (since it is an API limitation). So a printf
should be fine here.
  

Hi Lukasz,

> This patch provides test for utime. It uses xfstat() syscall to read
> access and modification times to compare with ones written by utime.
> 
> Moreover, access and modification times beyond the Y2038 threshold
> date (i.e. 32 bit time_t overflow) are also checked.

It seems that this and the utimes/futimens tests fail on machines where
/tmp is XFS, where they run into a y2038 bug [1].

I am wondering if there's a way around this, like maybe using a loop mount
inside a container? I don't know if that's possible/easy using the
container tests infrastructure, but it is the first thing that comes to my
mind.

Cheers,
Arjun

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1795576
  

* Arjun Shankar:

> Hi Lukasz,
>
>> This patch provides test for utime. It uses xfstat() syscall to read
>> access and modification times to compare with ones written by utime.
>> 
>> Moreover, access and modification times beyond the Y2038 threshold
>> date (i.e. 32 bit time_t overflow) are also checked.
>
> It seems that this and the utimes/futimens tests fail on machines where
> /tmp is XFS, where they run into a y2038 bug [1].
>
> I am wondering if there's a way around this, like maybe using a loop mount
> inside a container? I don't know if that's possible/easy using the
> container tests infrastructure, but it is the first thing that comes to my
> mind.

If tmpfs is known to work, using /dev/shm instead may be an option.

Thanks,
Florian
  

Hi Florian, Arjun,

> * Arjun Shankar:
> 
> > Hi Lukasz,
> >  
> >> This patch provides test for utime. It uses xfstat() syscall to
> >> read access and modification times to compare with ones written by
> >> utime.
> >> 
> >> Moreover, access and modification times beyond the Y2038 threshold
> >> date (i.e. 32 bit time_t overflow) are also checked.  
> >
> > It seems that this and the utimes/futimens tests fail on machines
> > where /tmp is XFS, where they run into a y2038 bug [1].
> >
> > I am wondering if there's a way around this, like maybe using a
> > loop mount inside a container? I don't know if that's possible/easy
> > using the container tests infrastructure, but it is the first thing
> > that comes to my mind.  
> 
> If tmpfs is known to work, using /dev/shm instead may be an option.
> 

I must admit that I've only tested this test with /tmp mounted as tmpfs.

As Florian pointed out - maybe /dev/shm would be an option...

> Thanks,
> Florian
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On 04/03/2021 13:17, Arjun Shankar wrote:
> Hi Lukasz,
> 
>> This patch provides test for utime. It uses xfstat() syscall to read
>> access and modification times to compare with ones written by utime.
>>
>> Moreover, access and modification times beyond the Y2038 threshold
>> date (i.e. 32 bit time_t overflow) are also checked.
> 
> It seems that this and the utimes/futimens tests fail on machines where
> /tmp is XFS, where they run into a y2038 bug [1].
> 
> I am wondering if there's a way around this, like maybe using a loop mount
> inside a container? I don't know if that's possible/easy using the
> container tests infrastructure, but it is the first thing that comes to my
> mind.
> 
> Cheers,
> Arjun
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1795576
> 

Maybe fix the kernel or/and use a non buggy FS instead? IMHO the test is
doing *exactly* is meant to do: trigger and expose an unexpected return
from the kernel or the libc. I don't think hiding it using a different
FS is the right thing to do on glibc testsuite.
  

* Adhemerval Zanella:

> Maybe fix the kernel or/and use a non buggy FS instead? IMHO the test is
> doing *exactly* is meant to do: trigger and expose an unexpected return
> from the kernel or the libc. I don't think hiding it using a different
> FS is the right thing to do on glibc testsuite.

I disagree: We want to test glibc here, not file systems.  Test outputs
are already difficult enough to interpret.  We should not add known
FAILs that are very difficult to fix (reformat & reinstall with another
file system is definitely in that category).

Thanks,
Florian
  

On 05/03/2021 17:29, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Maybe fix the kernel or/and use a non buggy FS instead? IMHO the test is
>> doing *exactly* is meant to do: trigger and expose an unexpected return
>> from the kernel or the libc. I don't think hiding it using a different
>> FS is the right thing to do on glibc testsuite.
> 
> I disagree: We want to test glibc here, not file systems.  Test outputs
> are already difficult enough to interpret.  We should not add known
> FAILs that are very difficult to fix (reformat & reinstall with another
> file system is definitely in that category).

My take is since we are the first step to drive the proper y2038 support
and it has time limit, it is better to fail hard and signal something
is wrong either with glibc or kernel *now* so we iron out the issues
instead of fail once users actually deploy glibc on system that are meant
to handle y2038 issues.
  

* Adhemerval Zanella:

> On 05/03/2021 17:29, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> Maybe fix the kernel or/and use a non buggy FS instead? IMHO the test is
>>> doing *exactly* is meant to do: trigger and expose an unexpected return
>>> from the kernel or the libc. I don't think hiding it using a different
>>> FS is the right thing to do on glibc testsuite.
>> 
>> I disagree: We want to test glibc here, not file systems.  Test outputs
>> are already difficult enough to interpret.  We should not add known
>> FAILs that are very difficult to fix (reformat & reinstall with another
>> file system is definitely in that category).
>
> My take is since we are the first step to drive the proper y2038 support
> and it has time limit, it is better to fail hard and signal something
> is wrong either with glibc or kernel *now* so we iron out the issues
> instead of fail once users actually deploy glibc on system that are meant
> to handle y2038 issues.

We know precisely what is wrong with XFS (and the other file systems
which need to change their disk layout).  No one gains from testing this
during glibc builds.

Again, I do not suggest not to test this, but rather use a path which is
required to use a specific file system (tmpfs), so that the test outcome
is more predictable.  The glibc side and the higher VFS layers in the
kernel are still fully covered if we use tmpfs, so I do not think this
reduces coverage as far as glibc is concerned.

Thanks,
Florian
  

On 08/03/2021 10:46, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 05/03/2021 17:29, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> Maybe fix the kernel or/and use a non buggy FS instead? IMHO the test is
>>>> doing *exactly* is meant to do: trigger and expose an unexpected return
>>>> from the kernel or the libc. I don't think hiding it using a different
>>>> FS is the right thing to do on glibc testsuite.
>>>
>>> I disagree: We want to test glibc here, not file systems.  Test outputs
>>> are already difficult enough to interpret.  We should not add known
>>> FAILs that are very difficult to fix (reformat & reinstall with another
>>> file system is definitely in that category).
>>
>> My take is since we are the first step to drive the proper y2038 support
>> and it has time limit, it is better to fail hard and signal something
>> is wrong either with glibc or kernel *now* so we iron out the issues
>> instead of fail once users actually deploy glibc on system that are meant
>> to handle y2038 issues.
> 
> We know precisely what is wrong with XFS (and the other file systems
> which need to change their disk layout).  No one gains from testing this
> during glibc builds.

No, but indicates that your system need to be fixed if you intend to
deploy a y2038 binaries on it.

> 
> Again, I do not suggest not to test this, but rather use a path which is
> required to use a specific file system (tmpfs), so that the test outcome
> is more predictable.  The glibc side and the higher VFS layers in the
> kernel are still fully covered if we use tmpfs, so I do not think this
> reduces coverage as far as glibc is concerned.

What if kernel introduce a regression on tmpfs or any other filesystem,
should we gloss over to use a different filesystem instead? Or should we
add a routine to check for the filesystem support for y2038 and disable
or return unsupported instead?

Also, this issue will most likely get fixed way before y2038 will actually
be deployed. So I am not seeing the urgency here of avoid this kind of 
transient failures.
  

* Adhemerval Zanella:

> What if kernel introduce a regression on tmpfs or any other filesystem,
> should we gloss over to use a different filesystem instead?

tmpfs is different because there is no backwards compatibility
requirement for its data structures, so bugs can be fixed in a more
straightforward way than other file systems.

Thanks,
Florian