Re: [PATCH] tests: fix test-execute with GNU make jobserver

[Bruno Haible]

Hi Dmitry,

> Yes, it is reliable as it mirrors
>       for (fd = 0; fd < 20; fd++)
> piece of code in tests/test-execute-child.c file.

Ah, I had missed that. Thanks.

> there could
> be other external sources of inherited descriptors, so the test should
> rather be robust and make sure these external sources do not affect the
> test.

Yes, KDE/plasma and NetBSD are such external sources of inherited descriptors.

> I'd choose a simple close() rather than a bit more complex cloexec if
> tests/test-execute-child.c did not contain this uncommented hunk:
>         for (fd = 0; fd < 20; fd++)
>           #ifdef __NetBSD__
>           if (fd != 3)
>           #endif
> 
> If for some reason __NetBSD__ is allowed to have descriptor #3 inherited,
> then it shouldn't be closed in tests/test-execute-main.c either.

That was just my first attempt at working around the issue. It occurred
first on NetBSD.

> > 4) How about other environments? I saw 'test-execute.sh' also fail in
> > some recent Linux distro with KDE desktop. Apparently 'plasma' is passing
> > one or two open file descriptors to all children (that is, to all processes
> > running in this desktop environment). Do you think this is worth reporting?
> 
> If it isn't documented, then I'd say it's a bug, please report it.

Will do.

> > If so, Gnulib should probably provide a substitute close_range(), and
> > test-execute-main.c should better call this function, rather than rolling
> > its own loop.
> 
> Yes, I think it makes sense to provide this API in gnulib, although it
> won't be able to address all issues the syscall is intended to address.

I now read from [1] comment 23:
  "The close_range seems a sensible addition to added as a syscall wrapper
   for Linux, without any extra fallback, and returning ENOSYS on older
   kernel (as for copy_file_range)."
So, Gnulib would have to provide a function that invokes close_range AND
has fallback code. (That is general philosophy of Gnulib: provide
easily usable functions, rather than very low-level bricks that require
extensive manpage consultation before use.)

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=10353

I'm applying this fix:


2021-04-07  Bruno Haible  <bruno@clisp.org>

        execute tests: Avoid test failure in certain environments.
        Reported by Dmitry V. Levin <ldv@altlinux.org> in
        <https://lists.gnu.org/archive/html/bug-gnulib/2021-04/msg00082.html>.
        * tests/test-execute-main.c (main): Close file descriptors 3..19.
        * tests/test-execute-child.c (main): Remove NetBSD workaround.
        * modules/execute-tests (configure.ac): Test for close_range function.

diff --git a/modules/execute-tests b/modules/execute-tests
index 422c1ab..c942130 100644
--- a/modules/execute-tests
+++ b/modules/execute-tests
@@ -18,6 +18,8 @@ stdint
 unistd
 
 configure.ac:
+dnl Test for Linux system call close_range(), declared in <unistd.h>.
+AC_CHECK_FUNCS([close_range])
 
 Makefile.am:
 TESTS += test-execute.sh
diff --git a/tests/test-execute-child.c b/tests/test-execute-child.c
index c3a1190..dbaae1e 100644
--- a/tests/test-execute-child.c
+++ b/tests/test-execute-child.c
@@ -171,14 +171,11 @@ main (int argc, char *argv[])
         char *p = buf;
         int fd;
         for (fd = 0; fd < 20; fd++)
-          #ifdef __NetBSD__
-          if (fd != 3)
-          #endif
-            if (is_open (fd))
-              {
-                sprintf (p, "%d ", fd);
-                p += strlen (p);
-              }
+          if (is_open (fd))
+            {
+              sprintf (p, "%d ", fd);
+              p += strlen (p);
+            }
         const char *expected = (test < 16 ? "0 1 2 10 " : "0 1 2 ");
         if (strcmp (buf, expected) == 0)
           return 0;
diff --git a/tests/test-execute-main.c b/tests/test-execute-main.c
index 39e24af..944dcd1 100644
--- a/tests/test-execute-main.c
+++ b/tests/test-execute-main.c
@@ -58,6 +58,31 @@ main (int argc, char *argv[])
   char *prog_path = argv[1];
   const char *progname = "test-execute-child";
   int test = atoi (argv[2]);
+
+  switch (test)
+    {
+    case 14:
+    case 15:
+    case 16:
+      /* Close file descriptors that have been inherited from the parent
+         process and that would cause failures in test-execute-child.c.
+         Such file descriptors have been seen:
+           - with GNU make, when invoked as 'make -j N' with j > 1,
+           - in some versions of the KDE desktop environment,
+           - on NetBSD.
+       */
+      #if HAVE_CLOSE_RANGE
+      if (close_range (3, 20 - 1, 0) < 0)
+      #endif
+        {
+          int fd;
+          for (fd = 3; fd < 20; fd++)
+            close (fd);
+        }
+    default:
+      break;
+    }
+
   switch (test)
     {
     case 0: