[PATCH] fts: make debug version compilable again

[PATCH] fts: make debug version compilable again

[Paul Eggert]

This fixes things in what I hope is a better way than the
fd-only approach proposed by Kamil Dudka here:
https://lists.gnu.org/archive/html/bug-gnulib/2018-03/msg00079.html
https://lists.gnu.org/archive/html/bug-gnulib/2018-03/msg00080.html
* lib/fts.c (GNULIB_FTS_DEBUG): Rename from FTS_DEBUG,
to avoid collision with coreutils symbol.
Do not include <stdint.h> (not needed, since we include <stdint.h>)
or "getcwdat.h" (no longer exists).
(fd_ring_check, fd_ring_print): Add forward decls.
(struct devino): New type.
(PRINT_DEVINO): New macro.
(getdevino): New static function.
(fd_ring_print): Do nothing if not debugging.
(fd_ring_print, fd_ring_check): Use getdevino instead of getcwdat.
The output isn’t as good, but at least it compiles and runs.
---
 ChangeLog | 19 ++++++++++++++++++
 lib/fts.c | 58 ++++++++++++++++++++++++++++++++++---------------------
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bd0ca3592c..d5683cdd4c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2022-12-09  Paul Eggert  <eggert@cs.ucla.edu>
+
+	fts: make debug version compilable again
+	This fixes things in what I hope is a better way than the
+	fd-only approach proposed by Kamil Dudka here:
+	https://lists.gnu.org/archive/html/bug-gnulib/2018-03/msg00079.html
+	https://lists.gnu.org/archive/html/bug-gnulib/2018-03/msg00080.html
+	* lib/fts.c (GNULIB_FTS_DEBUG): Rename from FTS_DEBUG,
+	to avoid collision with coreutils symbol.
+	Do not include <stdint.h> (not needed, since we include <stdint.h>)
+	or "getcwdat.h" (no longer exists).
+	(fd_ring_check, fd_ring_print): Add forward decls.
+	(struct devino): New type.
+	(PRINT_DEVINO): New macro.
+	(getdevino): New static function.
+	(fd_ring_print): Do nothing if not debugging.
+	(fd_ring_print, fd_ring_check): Use getdevino instead of getcwdat.
+	The output isn’t as good, but at least it compiles and runs.
+
 2022-12-07  Paul Eggert  <eggert@cs.ucla.edu>
 
 	verify: update __STDC_VERSION__ as per C23
diff --git a/lib/fts.c b/lib/fts.c
index 74a08f7ec8..2627a75e43 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -252,13 +252,13 @@ static int      fts_safe_changedir (FTS *, FTSENT *, int, const char *)
 #define BNAMES          2               /* fts_children, names only */
 #define BREAD           3               /* fts_read */
 
-#if FTS_DEBUG
+#if GNULIB_FTS_DEBUG
 # include <inttypes.h>
-# include <stdint.h>
 # include <stdio.h>
-# include "getcwdat.h"
 bool fts_debug = false;
 # define Dprintf(x) do { if (fts_debug) printf x; } while (false)
+static void fd_ring_check (FTS const *);
+static void fd_ring_print (FTS const *, FILE *, char const *);
 #else
 # define Dprintf(x)
 # define fd_ring_check(x)
@@ -1629,7 +1629,23 @@ mem1:                           saved_errno = errno;
         return (head);
 }
 
-#if FTS_DEBUG
+#if GNULIB_FTS_DEBUG
+
+struct devino {
+  intmax_t dev, ino;
+};
+#define PRINT_DEVINO "(%jd,%jd)"
+
+static struct devino
+getdevino (int fd)
+{
+  struct stat st;
+  return (fd == AT_FDCWD
+          ? (struct devino) { -1, 0 }
+          : fstat (fd, &st) == 0
+          ? (struct devino) { st.st_dev, st.st_ino }
+          : (struct devino) { -1, errno });
+}
 
 /* Walk ->fts_parent links starting at E_CURR, until the root of the
    current hierarchy.  There should be a directory with dev/inode
@@ -1703,26 +1719,26 @@ same_fd (int fd1, int fd2)
 static void
 fd_ring_print (FTS const *sp, FILE *stream, char const *msg)
 {
+  if (!fts_debug)
+    return;
   I_ring const *fd_ring = &sp->fts_fd_ring;
-  unsigned int i = fd_ring->fts_front;
-  char *cwd = getcwdat (sp->fts_cwd_fd, NULL, 0);
-  fprintf (stream, "=== %s ========== %s\n", msg, cwd);
-  free (cwd);
+  unsigned int i = fd_ring->ir_front;
+  struct devino cwd = getdevino (sp->fts_cwd_fd);
+  fprintf (stream, "=== %s ========== "PRINT_DEVINO"\n", msg, cwd.dev, cwd.ino);
   if (i_ring_empty (fd_ring))
     return;
 
   while (true)
     {
-      int fd = fd_ring->fts_fd_ring[i];
+      int fd = fd_ring->ir_data[i];
       if (fd < 0)
         fprintf (stream, "%d: %d:\n", i, fd);
       else
         {
-          char *wd = getcwdat (fd, NULL, 0);
-          fprintf (stream, "%d: %d: %s\n", i, fd, wd);
-          free (wd);
+          struct devino wd = getdevino (fd);
+          fprintf (stream, "%d: %d: "PRINT_DEVINO"\n", i, fd, wd.dev, wd.ino);
         }
-      if (i == fd_ring->fts_back)
+      if (i == fd_ring->ir_back)
         break;
       i = (i + I_RING_SIZE - 1) % I_RING_SIZE;
     }
@@ -1741,9 +1757,9 @@ fd_ring_check (FTS const *sp)
 
   int cwd_fd = sp->fts_cwd_fd;
   cwd_fd = fcntl (cwd_fd, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
-  char *dot = getcwdat (cwd_fd, NULL, 0);
-  error (0, 0, "===== check ===== cwd: %s", dot);
-  free (dot);
+  struct devino dot = getdevino (cwd_fd);
+  fprintf (stderr, "===== check ===== cwd: "PRINT_DEVINO"\n",
+           dot.dev, dot.ino);
   while ( ! i_ring_empty (&fd_w))
     {
       int fd = i_ring_pop (&fd_w);
@@ -1758,12 +1774,10 @@ fd_ring_check (FTS const *sp)
             }
           if (!same_fd (fd, parent_fd))
             {
-              char *cwd = getcwdat (fd, NULL, 0);
-              error (0, errno, "ring  : %s", cwd);
-              char *c2 = getcwdat (parent_fd, NULL, 0);
-              error (0, errno, "parent: %s", c2);
-              free (cwd);
-              free (c2);
+              struct devino cwd = getdevino (fd);
+              fprintf (stderr, "ring  : "PRINT_DEVINO"\n", cwd.dev, cwd.ino);
+              struct devino c2 = getdevino (parent_fd);
+              fprintf (stderr, "parent: "PRINT_DEVINO"\n", c2.dev, c2.ino);
               fts_assert (0);
             }
           close (cwd_fd);
-- 
2.38.1

fgetname function?

[Bruno Haible]

Paul Eggert wrote:
> https://lists.gnu.org/archive/html/bug-gnulib/2018-03/msg00079.html
> https://lists.gnu.org/archive/html/bug-gnulib/2018-03/msg00080.html

This thread contains a posting by Jim Meyering, with an attachment
getcwdat.c, that contains a function for finding the name of a directory
given as a file descriptor.

I'm wondering if we should add a generalization of this function:

  char * fgetname (int fd, char *buf, size_t size);

that would return the current file name of a file descriptor, if
available.

This function would have limitations:
  - For non-directories, it would only work on specific operating
    systems (in particular, not on OpenBSD).
  - For directories, it may not work when an ancestor directory
    is not readable (same restriction as for getcwd).

Also, it would be prone to races, and thus discouraged for functional
code. But it might be a useful addition for debugging code.

The implementation would proceed as described in [1][2]:
  - On Linux, use readlink("/proc/self/fd/<fd>").
  - On macOS and NetBSD, use fcntl F_GETPATH.[3][4]
  - On native Windows, use GetFileInformationByHandleEx with FileNameInfo.[5]
  - On FreeBSD, use kinfo_getfile or sysctl KERN_PROC_FILEDESC [6].
  - Otherwise, on directories, use a loop, like in Jim's code:
      fd = openat (fd, "..", O_RDONLY);

I think it would be worth it, because nowadays there are few OSes
where this would not work (e.g. OpenBSD).

What do think?

Bruno

[1] https://stackoverflow.com/questions/1188757/
[2] https://www.wikitechy.com/tutorials/linux/how-to-get-filename-from-file-descriptor-linux-in-c
[3] https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fcntl.2.html
[4] https://man.netbsd.org/fcntl.2
[5] https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex
[6] https://www.freebsd.org/cgi/man.cgi?apropos=0&sektion=3&query=kinfo_getfile&manpath=FreeBSD+7.0-current&format=html

Re: fgetname function?

[Paul Eggert]

On 2022-12-11 18:41, Bruno Haible wrote:
> Also, it would be prone to races, and thus discouraged for functional
> code. But it might be a useful addition for debugging code.

Also, it wouldn't work for files that have never had a name (e.g., 
ordinary pipes) - unless we want to create a special syntax for nameless 
files, useful for debugging but not as file names (I assume we'd use 
GNU/Linux's syntax?). And similarly I suppose we'd replicate GNU/Linux 
behavior for nameless but formerly-named files that have been deleted 
but are still open. It could be a bit tricky with FDs open to symlinks, 
or with files that have been reattached into the file system, but I 
assume there are solutions to all that.

> it might be a useful addition for debugging code

If non-debugging modules don't depend on it, this sounds like it may 
well be useful for debugging.

I haven't felt much of a need for this sort of function myself, though I 
expect that's partly luck and perhaps partly my debugging style. For 
example, although fgetname would have been a nicety for the du issue 
prompting the recent Gnulib change, the most important thing was logging 
device and inode numbers; the names weren't as important since they were 
deducible from the other output.