diff mbox series

support: Enhance support_need_proc

Message ID 87ldx1kens.fsf@oldenburg.str.redhat.com
State Under Review
Delegated to: Adhemerval Zanella Netto
Headers
Series support: Enhance support_need_proc |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
redhat-pt-bot/TryBot-32bit success Build for i686

Commit Message

Florian Weimer Nov. 29, 2024, 7:45 p.m. UTC 
  Explicitly specify the name of the required file.

The xmkdirp-related check in nss/tst-reload2 looks unrelated to /proc,
so remove it.

DJ, do you know why it is there?  Is it perhaps checking for something
else by proxy?

Thanks,
Florian

---
 elf/tst-decorate-maps.c     |  2 +-
 elf/tst-pldd.c              |  2 +-
 nptl/tst-pthread-getattr.c  |  2 +-
 nss/tst-reload2.c           |  2 --
 support/support.h           |  8 ++++----
 support/support_need_proc.c | 13 +++----------
 6 files changed, 10 insertions(+), 19 deletions(-)


base-commit: bde47662b74b883149c3001e2c052dea5d3cd92f

Comments

DJ Delorie Dec. 2, 2024, 7:10 p.m. UTC | #1 
Florian Weimer <fweimer@redhat.com> writes:
> The xmkdirp-related check in nss/tst-reload2 looks unrelated to /proc,
> so remove it.
>
> DJ, do you know why it is there?  Is it perhaps checking for something
> else by proxy?

As noted in the sources, /proc must be present for the id mapping to
work, so that xmkdirp succeeds.  This is a container-related issue, and
tst-reload2 is a container test.
Florian Weimer Dec. 2, 2024, 7:38 p.m. UTC | #2 
* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> The xmkdirp-related check in nss/tst-reload2 looks unrelated to /proc,
>> so remove it.
>>
>> DJ, do you know why it is there?  Is it perhaps checking for something
>> else by proxy?
>
> As noted in the sources, /proc must be present for the id mapping to
> work, so that xmkdirp succeeds.  This is a container-related issue, and
> tst-reload2 is a container test.

I figured out this much from the comments, but it doesn't make quite
sense: I don't see anything in our xmkdirp implementation that would
require this.  And I would assume that directories and files aren't
different, and that a lack of user mapping would impact creation of
files in other places, too.

Thanks,
Florian
DJ Delorie Dec. 2, 2024, 7:42 p.m. UTC | #3 
Florian Weimer <fweimer@redhat.com> writes:
> I figured out this much from the comments, but it doesn't make quite
> sense: I don't see anything in our xmkdirp implementation that would
> require this.  And I would assume that directories and files aren't
> different, and that a lack of user mapping would impact creation of
> files in other places, too.

It's the generic "need perms to mkdir".  Checking for /proc and issuing
a meaningful error message is far more useful than xmkdir failing with
"xmkdir: permission denied" with no explaination *why*.

We used to fail all container tests if test-container couldn't mount
/proc but that was changed...
Florian Weimer Dec. 2, 2024, 8:12 p.m. UTC | #4 
* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> I figured out this much from the comments, but it doesn't make quite
>> sense: I don't see anything in our xmkdirp implementation that would
>> require this.  And I would assume that directories and files aren't
>> different, and that a lack of user mapping would impact creation of
>> files in other places, too.
>
> It's the generic "need perms to mkdir".  Checking for /proc and issuing
> a meaningful error message is far more useful than xmkdir failing with
> "xmkdir: permission denied" with no explaination *why*.
>
> We used to fail all container tests if test-container couldn't mount
> /proc but that was changed...

Are you able to reproduce it in some way?

Thanks,
Florian
DJ Delorie Dec. 2, 2024, 9:09 p.m. UTC | #5 
Florian Weimer <fweimer@redhat.com> writes:
>> We used to fail all container tests if test-container couldn't mount
>> /proc but that was changed...
>
> Are you able to reproduce it in some way?

Not easily, and I don't recall which of the many environments that run
glibc testing has the limitation needed.
diff mbox series

Patch

diff --git a/elf/tst-decorate-maps.c b/elf/tst-decorate-maps.c
index 6d04344ba2..509c8c6889 100644
--- a/elf/tst-decorate-maps.c
+++ b/elf/tst-decorate-maps.c
@@ -182,7 +182,7 @@  do_prepare (int argc, char *argv[])
 static int
 do_test (void)
 {
-  support_need_proc ("Reads /proc/self/maps to get stack names.");
+  support_need_proc ("/proc/self/maps", "Used to read map names");
 
   if (!support_set_vma_name_supported ())
     FAIL_UNSUPPORTED ("kernel does not support PR_SET_VMA_ANON_NAME");
diff --git a/elf/tst-pldd.c b/elf/tst-pldd.c
index 512481c17f..2a5e340f8a 100644
--- a/elf/tst-pldd.c
+++ b/elf/tst-pldd.c
@@ -85,7 +85,7 @@  in_str_list (const char *libname, const char *const strlist[])
 static int
 do_test (void)
 {
-  support_need_proc ("needs /proc/sys/kernel/yama/ptrace_scope and /proc/$child");
+  support_need_proc ("/proc/self", "YAMA scope check requires mounted /proc");
 
   /* Check if our subprocess can be debugged with ptrace.  */
   {
diff --git a/nptl/tst-pthread-getattr.c b/nptl/tst-pthread-getattr.c
index 8283198389..bbefa53f03 100644
--- a/nptl/tst-pthread-getattr.c
+++ b/nptl/tst-pthread-getattr.c
@@ -155,7 +155,7 @@  check_stack_top (void)
 static int
 do_test (void)
 {
-  support_need_proc ("Reads /proc/self/maps to get stack size.");
+  support_need_proc ("/proc/self/maps", "Used to obtain stack size");
 
   pagesize = sysconf (_SC_PAGESIZE);
   return check_stack_top ();
diff --git a/nss/tst-reload2.c b/nss/tst-reload2.c
index ccc562254c..dd19005c58 100644
--- a/nss/tst-reload2.c
+++ b/nss/tst-reload2.c
@@ -95,8 +95,6 @@  do_test (void)
   char buf1[PATH_MAX];
   char buf2[PATH_MAX];
 
-  support_need_proc ("Our xmkdirp fails if we can't map our uid, which requires /proc.");
-
   sprintf (buf1, "/subdir%s", support_slibdir_prefix);
   xmkdirp (buf1, 0777);
 
diff --git a/support/support.h b/support/support.h
index ba21ec9b5a..8ea7c44ee5 100644
--- a/support/support.h
+++ b/support/support.h
@@ -91,10 +91,10 @@  char *support_quote_string (const char *);
    regular file open for writing, and initially empty.  */
 int support_descriptor_supports_holes (int fd);
 
-/* Predicates that a test requires a working /proc filesystem.  This
-   call will exit with UNSUPPORTED if /proc is not available, printing
-   WHY_MSG as part of the diagnostic.  */
-void support_need_proc (const char *why_msg);
+/* Predicates that a test requires PATH (which must start with /proc).
+   This call will exit with UNSUPPORTED if PATH is not available,
+   printing WHY_MSG as part of the diagnostic.  */
+void support_need_proc (const char *path, const char *why_msg);
 
 /* Error-checking wrapper functions which terminate the process on
    error.  */
diff --git a/support/support_need_proc.c b/support/support_need_proc.c
index 53356ddac1..b8b898ff24 100644
--- a/support/support_need_proc.c
+++ b/support/support_need_proc.c
@@ -20,16 +20,9 @@ 
 #include <support/check.h>
 #include <support/support.h>
 
-/* We test for /proc/self/maps since that's one of the files that one
-   of our tests actually uses, but the general idea is if Linux's
-   /proc/ (procfs) filesystem is mounted.  If not, the process exits
-   with an UNSUPPORTED result code.  */
-
 void
-support_need_proc (const char *why_msg)
+support_need_proc (const char *path, const char *why_msg)
 {
-#ifdef __linux__
-  if (access ("/proc/self/maps", R_OK))
-    FAIL_UNSUPPORTED ("/proc is not available, %s", why_msg);
-#endif
+  if (access (path, R_OK))
+    FAIL_UNSUPPORTED ("Missing %s: %s", path, why_msg);
 }