[PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Arjun Shankar]

From: Arjun Shankar <arjun@redhat.com>

tst-utime.c, tst-utimes.c, and tst-futimens.c create a temporary file in
/tmp to test Y2038 support in the corresponding functions.  This causes
the tests to fail when /tmp is mounted as a filesystem that has a Y2038
bug; e.g. XFS.

This commit changes test-container.c to mount /tmp as a tmpfs filesystem
and moves the utime tests to the tests-container target so that they run
in a container against a known-good filesystem.

The test-container change of mounting /tmp as tmpfs led to one of the
existing container tests, elf/tst-ldconfig-ld_so_conf-update, to fail.
This commit also adjusts the test to not use /tmp any more.
---
Notes:

v1 ("Prefer /dev/shm over /tmp in utime family tests") here:
https://sourceware.org/pipermail/libc-alpha/2021-March/123486.html

I went a different way this time.  Since we cannot touch /dev/shm and since
/tmp is not guaranteed to be tmpfs every time, I containerised the tests.
This meant modifying test-container to mount /tmp as tmpfs and adjusting one
existing container test that failed due to /tmp being a mount point.
---
 elf/tst-ldconfig-ld_so_conf-update.c | 4 ++--
 support/test-container.c             | 4 +++-
 sysdeps/unix/sysv/linux/Makefile     | 5 +++--
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/elf/tst-ldconfig-ld_so_conf-update.c b/elf/tst-ldconfig-ld_so_conf-update.c
index e8bd4c3598..7462516f59 100644
--- a/elf/tst-ldconfig-ld_so_conf-update.c
+++ b/elf/tst-ldconfig-ld_so_conf-update.c
@@ -32,7 +32,7 @@
 
 
 #define DSO "libldconfig-ld-mod.so"
-#define DSO_DIR "/tmp/tst-ldconfig"
+#define DSO_DIR "/tst-ldconfig"
 #define CONF "/etc/ld.so.conf"
 
 
@@ -73,7 +73,7 @@ do_test (void)
      "lib" (for regular shared libraries) or "ld-" (for ld-linux-*).  */
   char *mod_src_path = xasprintf ("%s/tst-ldconfig-ld-mod.so",
 				  support_libdir_prefix);
-  if (rename (mod_src_path, "/tmp/tst-ldconfig/libldconfig-ld-mod.so"))
+  if (rename (mod_src_path, DSO_DIR "/libldconfig-ld-mod.so"))
     FAIL_EXIT1 ("Renaming/moving the DSO failed: %m");
   free (mod_src_path);
 
diff --git a/support/test-container.c b/support/test-container.c
index 28cc44d9f1..0924bdc2e7 100644
--- a/support/test-container.c
+++ b/support/test-container.c
@@ -1160,7 +1160,9 @@ main (int argc, char **argv)
   sprintf (pid_buf, "%lu", (long unsigned)child);
   setenv ("PID_OUTSIDE_CONTAINER", pid_buf, 0);
 
-  maybe_xmkdir ("/tmp", 0755);
+  maybe_xmkdir ("/tmp", 1777);
+  if (mount ("none", "/tmp", "tmpfs", MS_NOEXEC | MS_NOSUID | MS_NODEV, NULL) < 0)
+    FAIL_EXIT1 ("can't mount tmpfs onto /tmp\n");
 
   /* Now that we're pid 1 (effectively "root") we can mount /proc  */
   maybe_xmkdir ("/proc", 0777);
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index e42bc7f13b..cdbeb81f9c 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -107,8 +107,9 @@ 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-utime tst-utimes \
-	 tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
+	 tst-timerfd tst-ppoll tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
+
+tests-container += tst-futimens tst-utime tst-utimes
 
 # Test for the symbol version of fcntl that was replaced in glibc 2.28.
 ifeq ($(have-GLIBC_2.27)$(build-shared),yesyes)
-- 
2.29.2

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[DJ Delorie]

Arjun Shankar <arjun.is@lostca.se> writes:
> -#define DSO_DIR "/tmp/tst-ldconfig"
> +#define DSO_DIR "/tst-ldconfig"
> -  if (rename (mod_src_path, "/tmp/tst-ldconfig/libldconfig-ld-mod.so"))
> +  if (rename (mod_src_path, DSO_DIR "/libldconfig-ld-mod.so"))

The net effect here is to just change the temporary directory we put the
DSO in, because we can't "rename" across filesystems.  Ok.

> -  maybe_xmkdir ("/tmp", 0755);
> +  maybe_xmkdir ("/tmp", 1777);

This change should be unneeded?  It creates a directory visible only
outside the container, which is world writable :-( Do we need this
change?  What perms does the tmpfs end up with?

(yes, I see we already use 0777 below ;)

> +  if (mount ("none", "/tmp", "tmpfs", MS_NOEXEC | MS_NOSUID | MS_NODEV, NULL) < 0)
> +    FAIL_EXIT1 ("can't mount tmpfs onto /tmp\n");

/tmp needs to be tmpfs inside the container, ok.

> -	 tst-timerfd tst-ppoll tst-futimens tst-utime tst-utimes \
> +	 tst-timerfd tst-ppoll tst-clock_adjtime tst-adjtimex tst-ntp_adjtime

> -	 tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
> +
> +tests-container += tst-futimens tst-utime tst-utimes

Moves three tests, ok.

LGTM but I have no opinion on the "we should test broken kernel
interfaces" debate ;-)

Reviewed-by: DJ Delorie <dj@redhat.com>

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Adhemerval Zanella]

On 09/03/2021 22:32, DJ Delorie via Libc-alpha wrote:
> Arjun Shankar <arjun.is@lostca.se> writes:
>> -#define DSO_DIR "/tmp/tst-ldconfig"
>> +#define DSO_DIR "/tst-ldconfig"
>> -  if (rename (mod_src_path, "/tmp/tst-ldconfig/libldconfig-ld-mod.so"))
>> +  if (rename (mod_src_path, DSO_DIR "/libldconfig-ld-mod.so"))
> 
> The net effect here is to just change the temporary directory we put the
> DSO in, because we can't "rename" across filesystems.  Ok.
> 
>> -  maybe_xmkdir ("/tmp", 0755);
>> +  maybe_xmkdir ("/tmp", 1777);
> 
> This change should be unneeded?  It creates a directory visible only
> outside the container, which is world writable :-( Do we need this
> change?  What perms does the tmpfs end up with?
> 
> (yes, I see we already use 0777 below ;)
> 
>> +  if (mount ("none", "/tmp", "tmpfs", MS_NOEXEC | MS_NOSUID | MS_NODEV, NULL) < 0)
>> +    FAIL_EXIT1 ("can't mount tmpfs onto /tmp\n");
> 
> /tmp needs to be tmpfs inside the container, ok.
> 
>> -	 tst-timerfd tst-ppoll tst-futimens tst-utime tst-utimes \
>> +	 tst-timerfd tst-ppoll tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
> 
>> -	 tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
>> +
>> +tests-container += tst-futimens tst-utime tst-utimes
> 
> Moves three tests, ok.
> 
> LGTM but I have no opinion on the "we should test broken kernel
> interfaces" debate ;-)

Sorry, I still think we should not gloss over buggy filesystems. 
I really want this tests to be tested on multiple different filesystem
and check if the interface is really working as intended.  There is no
point in providing y2038 interfaces that might be broken when used
on different FS that is actually used in the wild.

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Arjun Shankar]

Hi Adhemerval,

> Sorry, I still think we should not gloss over buggy filesystems.
> I really want this tests to be tested on multiple different filesystem
> and check if the interface is really working as intended.  There is no
> point in providing y2038 interfaces that might be broken when used
> on different FS that is actually used in the wild.

Testing various filesystems is certainly of value when done
systematically. The issues I see right now are:

In its present state, this test can fail even though there is no known
glibc bug in the functionality it tests. This makes the test flaky
when it comes to verifying glibc functionality itself.

In the future, this test could also fail because of a glibc regression.
At that point someone used to running the test and seeing it fail
often in the past could disregard the failure, or the filesystem bug
could mask a glibc regression, thus causing us to miss catching the
glibc regression. So this test is also flaky when it comes to catching
a potential regression.

A test that doesn't always pass when the glibc interface under test is
actually working well and doesn't always clearly identify a regression
because it is masked by a well known bug in some other component is a
test that has potential for improvement.

The way I understand it, this is a unit test of a glibc interface. The
goal of unit testing is to isolate each part of the program and show
that the individual parts are correct. In that spirit, I feel that
running this test in isolation from the filesystem has value and is an
improvement over its present state.

The XFS bug is well known. I feel that the value to this community
from consistently verifying glibc functionality is higher than the
value of reminding ourselves of a well known bug in another component.

I hope this makes a convincing argument, but irrespective of whether
you approve of this patch or the direction it takes: thank you for the
time you have taken to respond to these patches.

Cheers,
Arjun

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Mike Frysinger]

On 10 Mar 2021 09:33, Adhemerval Zanella via Libc-alpha wrote:
> On 09/03/2021 22:32, DJ Delorie via Libc-alpha wrote:
> > Arjun Shankar <arjun.is@lostca.se> writes:
> >> -#define DSO_DIR "/tmp/tst-ldconfig"
> >> +#define DSO_DIR "/tst-ldconfig"
> >> -  if (rename (mod_src_path, "/tmp/tst-ldconfig/libldconfig-ld-mod.so"))
> >> +  if (rename (mod_src_path, DSO_DIR "/libldconfig-ld-mod.so"))
> > 
> > The net effect here is to just change the temporary directory we put the
> > DSO in, because we can't "rename" across filesystems.  Ok.
> > 
> >> -  maybe_xmkdir ("/tmp", 0755);
> >> +  maybe_xmkdir ("/tmp", 1777);
> > 
> > This change should be unneeded?  It creates a directory visible only
> > outside the container, which is world writable :-( Do we need this
> > change?  What perms does the tmpfs end up with?
> > 
> > (yes, I see we already use 0777 below ;)
> > 
> >> +  if (mount ("none", "/tmp", "tmpfs", MS_NOEXEC | MS_NOSUID | MS_NODEV, NULL) < 0)
> >> +    FAIL_EXIT1 ("can't mount tmpfs onto /tmp\n");
> > 
> > /tmp needs to be tmpfs inside the container, ok.
> > 
> >> -	 tst-timerfd tst-ppoll tst-futimens tst-utime tst-utimes \
> >> +	 tst-timerfd tst-ppoll tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
> > 
> >> -	 tst-clock_adjtime tst-adjtimex tst-ntp_adjtime
> >> +
> >> +tests-container += tst-futimens tst-utime tst-utimes
> > 
> > Moves three tests, ok.
> > 
> > LGTM but I have no opinion on the "we should test broken kernel
> > interfaces" debate ;-)
> 
> Sorry, I still think we should not gloss over buggy filesystems. 
> I really want this tests to be tested on multiple different filesystem
> and check if the interface is really working as intended.  There is no
> point in providing y2038 interfaces that might be broken when used
> on different FS that is actually used in the wild.

test failures are going to confuse people and make them investigate.  i
think it's reasonable to have the test do some cursory prerequisite checks
to flag the test as SKIP or XFAIL, assuming it can be done cheaply.  then
devs can quickly determine the source of the problem and then ignore it,
or change their build env to handle it.

i'm not saying this particular patch is the best way to address the KIs.
-mike

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Adhemerval Zanella]

On 10/03/2021 11:13, Arjun Shankar wrote:
> Hi Adhemerval,
> 
>> Sorry, I still think we should not gloss over buggy filesystems.
>> I really want this tests to be tested on multiple different filesystem
>> and check if the interface is really working as intended.  There is no
>> point in providing y2038 interfaces that might be broken when used
>> on different FS that is actually used in the wild.
> 
> Testing various filesystems is certainly of value when done
> systematically. The issues I see right now are:
> 
> In its present state, this test can fail even though there is no known
> glibc bug in the functionality it tests. This makes the test flaky
> when it comes to verifying glibc functionality itself.
> 
> In the future, this test could also fail because of a glibc regression.
> At that point someone used to running the test and seeing it fail
> often in the past could disregard the failure, or the filesystem bug
> could mask a glibc regression, thus causing us to miss catching the
> glibc regression. So this test is also flaky when it comes to catching
> a potential regression.
> 
> A test that doesn't always pass when the glibc interface under test is
> actually working well and doesn't always clearly identify a regression
> because it is masked by a well known bug in some other component is a
> test that has potential for improvement.
> 
> The way I understand it, this is a unit test of a glibc interface. The
> goal of unit testing is to isolate each part of the program and show
> that the individual parts are correct. In that spirit, I feel that
> running this test in isolation from the filesystem has value and is an
> improvement over its present state.

We had in the past multiple occasion where buggy kernel interface triggered
invalid results on valid tests:

  * On systems with Linux kernel 3.11 through 3.17, missing a backport of 
    commit 69a91c237ab0ebe4e9fdeaf6d0090c85275594ec (present in 3.18, 
    backports may be in some older stable series), io/tst-open-tmpfile 
    fails. [1]

  * posix/tst-posix_fadvise64 failure results from a kernel bug with 32-bit
    powerpc kernels [2] [3].

  * Various issues reported on WSL with 2.28 [2].

  * Linux between 4.13 and 4.15 return EOVERFLOW for LFS OFD locks usage 
    in compat mode (non-LFS ABI running on a LFS default kernel, such as 
    i386 on a x86_64 kernel or s390-32 on a s390-64 kernel) [4] [5].

What we have done in such cases, and I think it was the correct way,
is to *proper* document on each scenario the failure happen, the expected
results, and if applicable a way to avoid it.  If we start to add tests
exception for each failure, it will complicate the code base and restrict
the test itself (for instance, moving the y2038 to a container would 
actually check them on system/architecture/kernel without container support).

But the whole idea here is *not* check only the glibc interface, otherwise
we would make the test act a syscall wrapper where the syscall is not 
relly exercised (which is indeed possible with some ptrace tricks).

> 
> The XFS bug is well known. I feel that the value to this community
> from consistently verifying glibc functionality is higher than the
> value of reminding ourselves of a well known bug in another component.
> 
> I hope this makes a convincing argument, but irrespective of whether
> you approve of this patch or the direction it takes: thank you for the
> time you have taken to respond to these patches.

Sorry, but still think we should keep the tests as-is and keep failing
them on the buggy interface. As I said initially, they are doing *exactly*
what they are suppose to do: expose a bug in the functionality that glibc
is meant to provide to the users.

This is very similar to a similar Red Hat issue [6], but back then it
was marked as CLOSED/NOTABUG since "we don't have any work to do in
glibc for this bug".  I think we should thread this issue similar to
this one (although back then was a missing backports, instead of upstream
kernel bug; but I think the principle applies).

> 
> Cheers,
> Arjun
> 

[1] https://sourceware.org/glibc/wiki/Release/2.32
[2] https://sourceware.org/glibc/wiki/Release/2.29
[3] https://lkml.org/lkml/2017/1/4/680
[4] https://sourceware.org/glibc/wiki/Release/2.28
[5] https://sourceware.org/ml/libc-alpha/2018-07/msg00243.html
[6] https://bugzilla.redhat.com/show_bug.cgi?id=1647483

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Siddhesh Poyarekar]

On 3/10/21 8:26 PM, Adhemerval Zanella via Libc-alpha wrote:
> But the whole idea here is *not* check only the glibc interface, otherwise
> we would make the test act a syscall wrapper where the syscall is not
> relly exercised (which is indeed possible with some ptrace tricks).

I wonder how scalable or reliable this is though; it seems to depend on 
odd chance to discover failures in interfaces through glibc tests.  I 
reckon the glibc tests are there to test glibc functionality and 
controlling the environment to ensure that the tests are repeatable and 
reliable doesn't seem like a goal at odds with the project.

That said, I agree glibc doesn't live in a vacuum and maybe what we're 
looking at here is something on the lines of LTP.  That seems like a 
much better place to depend on broad (i.e. cross filesystem and kernel 
environment) coverage of functionality.

Siddhesh

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[DJ Delorie]

Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
> Sorry, I still think we should not gloss over buggy filesystems. 

The right solution to this is to mount one of every filesystem and test
them all, not leave it up to the randomness of /tmp and have to try to
deduce if it's a known bug or a new bug.

Oh wait, that's kernel testing.

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Adhemerval Zanella]

On 10/03/2021 15:31, DJ Delorie wrote:
> Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
>> Sorry, I still think we should not gloss over buggy filesystems. 
> 
> The right solution to this is to mount one of every filesystem and test
> them all, not leave it up to the randomness of /tmp and have to try to
> deduce if it's a known bug or a new bug.
> 
> Oh wait, that's kernel testing.
> 

This is similar to libidn2 requirement [1], where it has started to dump
FAILs if the system did not provide an updated library version. Carlos
back then [2] added that it should continue to dump a 'FAIL' to indicate 
'you have bugs in your system libidn'.

Back then I complained more that it was not advertised the possible
failures on the commit message nor in the patch discussion [3] (since
different than this situation, we knew beforehand that the tests would
start to fail on some systems).  Florian in the end added a note on 
2.28 release wiki [4].

So maybe it would be better to do something similar as Joseph did to
proper handle it [5] and mark the tests unsupported if the '/tmp'
is XFS or any buggy one.

[1] https://sourceware.org/pipermail/libc-alpha/2018-May/094072.html
[2] https://sourceware.org/pipermail/libc-alpha/2018-May/094075.html
[3] 7f9f1ecb710eac4d65bb02785ddf288cac098323
[4] https://sourceware.org/glibc/wiki/Release/2.28#libidn2_is_a_recommended_dependency
[5] 42f527c89dabfee80c674ffe6a498a665c6d8281

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[DJ Delorie]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> So maybe it would be better to do something similar as Joseph did to
> proper handle it [5] and mark the tests unsupported if the '/tmp'
> is XFS or any buggy one.

If it were as easy to detect and XFS mount as to check idn2's version
(one function call), and as difficult to access a working idn2
interface, I'd say sure, xfail it.

But in this case, we have a way of testing our interface independently
of kernel bugs - mounting tmpfs gives us a way to *validly* test our
code on a *known good* kernel module.  I don't think we should throw
away that test.

We're allowed to have two tests...

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Adhemerval Zanella]

On 10/03/2021 16:00, DJ Delorie wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> So maybe it would be better to do something similar as Joseph did to
>> proper handle it [5] and mark the tests unsupported if the '/tmp'
>> is XFS or any buggy one.
> 
> If it were as easy to detect and XFS mount as to check idn2's version
> (one function call), and as difficult to access a working idn2
> interface, I'd say sure, xfail it.

$ cat test.c 
#include <stdio.h>
#include <stdbool.h>
#include <linux/magic.h>
#include <sys/vfs.h>

static bool is_xfs (const char *path)
{
  struct statfs fs;
  statfs (path, &fs);
  return fs.f_type == XFS_SUPER_MAGIC;
}

int main (int argc, char *argv[])
{
  if (argc < 2)
    return 0;

  printf ("is_xfs (%s): %s\n", argv[1],
          is_xfs (argv[1]) ? "yes" : "no");
}
$ gcc -Wall test.c -o test
$ dd if=/dev/zero of=loopbackfile.img bs=100M count=1
1+0 records in
1+0 records out
104857600 bytes (105 MB, 100 MiB) copied, 0,0602215 s, 1,7 GB/s
$ sudo losetup -fP loopbackfile.img 
$ mkfs.xfs loopbackfile.img 
meta-data=loopbackfile.img       isize=512    agcount=4, agsize=6400 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1
data     =                       bsize=4096   blocks=25600, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=1368, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
$ mkdir loopfs
$ sudo mount -o loop /dev/loop0 loopfs/
azanella@birita:/tmp$ ./test loopfs/
is_xfs (loopfs/): yes
azanella@birita:/tmp$ ./test /tmp
is_xfs (/tmp): no

> 
> But in this case, we have a way of testing our interface independently
> of kernel bugs - mounting tmpfs gives us a way to *validly* test our
> code on a *known good* kernel module.  I don't think we should throw
> away that test.
> 
> We're allowed to have two tests...
> 

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[DJ Delorie]

Testing something in patchwork, please ignore.

Or reply saying "No!" as you please ;-)

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[DJ Delorie]

Another patchwork test, ignore or not at your peril ;-)

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Carlos O'Donell]

On 3/10/21 1:53 PM, Adhemerval Zanella via Libc-alpha wrote:
> On 10/03/2021 15:31, DJ Delorie wrote:
>> Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
>>> Sorry, I still think we should not gloss over buggy filesystems. 
>>
>> The right solution to this is to mount one of every filesystem and test
>> them all, not leave it up to the randomness of /tmp and have to try to
>> deduce if it's a known bug or a new bug.
>>
>> Oh wait, that's kernel testing.
>>
> 
> This is similar to libidn2 requirement [1], where it has started to dump
> FAILs if the system did not provide an updated library version. Carlos
> back then [2] added that it should continue to dump a 'FAIL' to indicate 
> 'you have bugs in your system libidn'.

We are smearing the definition of "testing" across two boundaries:

* integrated system testing (test glibc with the host and host libraries)

* unit testing (smallest logical piece to test depends on our definitions)

If we want a glibc "integrated system test" that verifies the current glibc
against the host /tmp, then we can have that.

I think that Arjun is arguing he would like to see the y2038 tests behave
more like unit tests and operate independently of the host.

I think that DJ has noted we could have 2 tests, one which covers the
whole system testing (or integration test) and a unit test.

In the case of libidn2, where glibc was calling out via dlopen to the
host libidn2, there was a good case to use the whole system testing
approach and FAIL the test if libidn2 was buggy. In order to make a unit
test out of that we'd need to bundle a known-good libidn2 or stub it out.

In the case of this kernel bug we can relatively easily clone a unit test
that uses tmpfs, and also run a whole system test, and provide comments
explaining why we have two tests.

> So maybe it would be better to do something similar as Joseph did to
> proper handle it [5] and mark the tests unsupported if the '/tmp'
> is XFS or any buggy one.
 
What if we did this?

(1) Add unit tests that use a container and tmpfs to test functionality.

(2) Refactor and use the same test but with the host /tmp and:
    - Look for XFS version that is buggy and mark it XFAIL.
    - Otherwise FAIL because it's a new kernel bug.

-- 
Cheers,
Carlos.

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Adhemerval Zanella]

On 11/03/2021 13:38, Carlos O'Donell wrote:
> On 3/10/21 1:53 PM, Adhemerval Zanella via Libc-alpha wrote:
>> On 10/03/2021 15:31, DJ Delorie wrote:
>>> Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
>>>> Sorry, I still think we should not gloss over buggy filesystems. 
>>>
>>> The right solution to this is to mount one of every filesystem and test
>>> them all, not leave it up to the randomness of /tmp and have to try to
>>> deduce if it's a known bug or a new bug.
>>>
>>> Oh wait, that's kernel testing.
>>>
>>
>> This is similar to libidn2 requirement [1], where it has started to dump
>> FAILs if the system did not provide an updated library version. Carlos
>> back then [2] added that it should continue to dump a 'FAIL' to indicate 
>> 'you have bugs in your system libidn'.
> 
> We are smearing the definition of "testing" across two boundaries:
> 
> * integrated system testing (test glibc with the host and host libraries)
> 
> * unit testing (smallest logical piece to test depends on our definitions)
> 
> If we want a glibc "integrated system test" that verifies the current glibc
> against the host /tmp, then we can have that.
> 
> I think that Arjun is arguing he would like to see the y2038 tests behave
> more like unit tests and operate independently of the host.
> 
> I think that DJ has noted we could have 2 tests, one which covers the
> whole system testing (or integration test) and a unit test.
> 
> In the case of libidn2, where glibc was calling out via dlopen to the
> host libidn2, there was a good case to use the whole system testing
> approach and FAIL the test if libidn2 was buggy. In order to make a unit
> test out of that we'd need to bundle a known-good libidn2 or stub it out.
> 
> In the case of this kernel bug we can relatively easily clone a unit test
> that uses tmpfs, and also run a whole system test, and provide comments
> explaining why we have two tests.
> 
>> So maybe it would be better to do something similar as Joseph did to
>> proper handle it [5] and mark the tests unsupported if the '/tmp'
>> is XFS or any buggy one.
>  
> What if we did this?
> 
> (1) Add unit tests that use a container and tmpfs to test functionality.
> 
> (2) Refactor and use the same test but with the host /tmp and:
>     - Look for XFS version that is buggy and mark it XFAIL.
>     - Otherwise FAIL because it's a new kernel bug.
> 

My main point is such tests are not *unit tests* itself because different
than other y2038 interface, like gmtime or like, they *do* require kernel
support to work as intended. And my second point is, since glibc would be
first gear that stress this out (since many other projects depends on
out 64 bit time support), make it explicit that the system has a buggy
interface is what the tests is meant to do.

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Carlos O'Donell]

On 3/11/21 12:16 PM, Adhemerval Zanella wrote:
> 
> 
> On 11/03/2021 13:38, Carlos O'Donell wrote:
>> On 3/10/21 1:53 PM, Adhemerval Zanella via Libc-alpha wrote:
>>> On 10/03/2021 15:31, DJ Delorie wrote:
>>>> Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
>>>>> Sorry, I still think we should not gloss over buggy filesystems. 
>>>>
>>>> The right solution to this is to mount one of every filesystem and test
>>>> them all, not leave it up to the randomness of /tmp and have to try to
>>>> deduce if it's a known bug or a new bug.
>>>>
>>>> Oh wait, that's kernel testing.
>>>>
>>>
>>> This is similar to libidn2 requirement [1], where it has started to dump
>>> FAILs if the system did not provide an updated library version. Carlos
>>> back then [2] added that it should continue to dump a 'FAIL' to indicate 
>>> 'you have bugs in your system libidn'.
>>
>> We are smearing the definition of "testing" across two boundaries:
>>
>> * integrated system testing (test glibc with the host and host libraries)
>>
>> * unit testing (smallest logical piece to test depends on our definitions)
>>
>> If we want a glibc "integrated system test" that verifies the current glibc
>> against the host /tmp, then we can have that.
>>
>> I think that Arjun is arguing he would like to see the y2038 tests behave
>> more like unit tests and operate independently of the host.
>>
>> I think that DJ has noted we could have 2 tests, one which covers the
>> whole system testing (or integration test) and a unit test.
>>
>> In the case of libidn2, where glibc was calling out via dlopen to the
>> host libidn2, there was a good case to use the whole system testing
>> approach and FAIL the test if libidn2 was buggy. In order to make a unit
>> test out of that we'd need to bundle a known-good libidn2 or stub it out.
>>
>> In the case of this kernel bug we can relatively easily clone a unit test
>> that uses tmpfs, and also run a whole system test, and provide comments
>> explaining why we have two tests.
>>
>>> So maybe it would be better to do something similar as Joseph did to
>>> proper handle it [5] and mark the tests unsupported if the '/tmp'
>>> is XFS or any buggy one.
>>  
>> What if we did this?
>>
>> (1) Add unit tests that use a container and tmpfs to test functionality.
>>
>> (2) Refactor and use the same test but with the host /tmp and:
>>     - Look for XFS version that is buggy and mark it XFAIL.
>>     - Otherwise FAIL because it's a new kernel bug.
>>
> 
> My main point is such tests are not *unit tests* itself because different
> than other y2038 interface, like gmtime or like, they *do* require kernel
> support to work as intended. And my second point is, since glibc would be
> first gear that stress this out (since many other projects depends on
> out 64 bit time support), make it explicit that the system has a buggy
> interface is what the tests is meant to do.
 No matter how we define "unit test" we still want maximum code testing coverage.

The best way to get that today is with this:

(a) Leave the tests as they are, but add XFS version checking for XFAIL.
    - Fails to test the code paths we built because of XFS.

(b) Add new tests that, instead of stubbing out functionality (like a unit
    test might do) just use an always known-good tmpfs /tmp.
    - Succeeds in increasing code coverage.

-- 
Cheers,
Carlos.

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Mike Frysinger]

On 11 Mar 2021 14:16, Adhemerval Zanella wrote:
> My main point is such tests are not *unit tests* itself because different
> than other y2038 interface, like gmtime or like, they *do* require kernel
> support to work as intended.

i understand the desire, but i don't think there's actual value in trying to
be "pure" in our definitions.

how does a test for an interface glibc exposes know if it's implemented
entirely in glibc, entirely by the OS, or some other combo or mechanism ?
we've designed the source layout specifically to allow transparent stacking
based on a number of factors (e.g. OS and architecture).  is the test
supposed to check whether some part of the implementation lives under an
"OS" directory?

if we cut out all the tests that require an OS (as in, a syscall), then we're
cutting real deep -- too deep.  plus, we *should* be validating how glibc is
calling the OS because we can easily (and have) used syscalls incorrectly.

it seems more like the debate is over how much effort we should be investing
in detecting known kernel bugs, and whether we should workaround or notify or
just let them fail.  trying to frame it as unit or integration or some other
term feels like a pointless semantic debate.

maybe we need a common "quirks" file or something.  when such a quirk is
detected, we can log it and point to a wiki page for the user.
-mike

Re: [PATCH v2] Mount /tmp as tmpfs in test-container and run utime tests in it

[Carlos O'Donell]

On 3/11/21 1:59 PM, Mike Frysinger wrote:
> On 11 Mar 2021 14:16, Adhemerval Zanella wrote:
>> My main point is such tests are not *unit tests* itself because different
>> than other y2038 interface, like gmtime or like, they *do* require kernel
>> support to work as intended.
> 
> i understand the desire, but i don't think there's actual value in trying to
> be "pure" in our definitions.
> 
> how does a test for an interface glibc exposes know if it's implemented
> entirely in glibc, entirely by the OS, or some other combo or mechanism ?
> we've designed the source layout specifically to allow transparent stacking
> based on a number of factors (e.g. OS and architecture).  is the test
> supposed to check whether some part of the implementation lives under an
> "OS" directory?
> 
> if we cut out all the tests that require an OS (as in, a syscall), then we're
> cutting real deep -- too deep.  plus, we *should* be validating how glibc is
> calling the OS because we can easily (and have) used syscalls incorrectly.
> 
> it seems more like the debate is over how much effort we should be investing
> in detecting known kernel bugs, and whether we should workaround or notify or
> just let them fail.  trying to frame it as unit or integration or some other
> term feels like a pointless semantic debate.

My point in highlighting the distinction is that there is room for testing both:
* A test that tests 64-bit time on tmpfs (unit test like with a more controlled impl)
* A test that tests 64-bit time on the host /tmp (integration test like)
 
> maybe we need a common "quirks" file or something.  when such a quirk is
> detected, we can log it and point to a wiki page for the user.

We should just XFAIL the test at runtime if we detect a broken fs.

Arjun just posted a patch to do just this and we should review that.

Adding more tests that use tmpfs is icing on the cake.

-- 
Cheers,
Carlos.