Subject: Re: bug#56710: ls vs. stat display of st_size

From: Paul Eggert <[email-masked]>
Date: Sat, 23 Jul 2022 13:07:28 -0700

On 7/23/22 05:17, Pádraig Brady wrote:

> BTW I see we've code in cache_fstatat() that assumes
> st_size can't have such large values, which contradicts a bit.

Good catch. I installed the first attached patch.

 > This is only a real consideration for virtual files I think
 > since off_t is signed, and so impractical for a real file system
 > to support files > OFF_T_MAX.

Yes, that sounds right.

You've convinced me that 'ls' should switch to the way 'stat' behaves 
rather than vice versa; that's more useful anyway. How about the 
attached second patch, which I haven't installed? (I was actually 
inclined this way originally but got lazy.)

From c2056a320b38126bf5566c2ce94e2c2b25243f66 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 23 Jul 2022 12:11:49 -0700
Subject: [PATCH 1/2] =?UTF-8?q?rm:=20don=E2=80=99t=20assume=20st=5Fsize=20?=
 =?UTF-8?q?is=20nonnegative?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/remove.c: Include stat-time.h.
(cache_fstatat, cache_stat_init): Use negative st->st_atim.tv_sec to
determine whether the stat is cached, not negative st->st_size.
On non-POSIX platforms that lack st_atim.tv_sec, don’t bother to cache.
---
 src/remove.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/remove.c b/src/remove.c
index b5d1ea8a2..e2f27ca4f 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -28,6 +28,7 @@
 #include "ignore-value.h"
 #include "remove.h"
 #include "root-dev-ino.h"
+#include "stat-time.h"
 #include "write-any-file.h"
 #include "xfts.h"
 #include "yesno.h"
@@ -62,29 +63,37 @@ enum Prompt_action
 # define DT_LNK 2
 #endif
 
-/* Like fstatat, but cache the result.  If ST->st_size is -1, the
-   status has not been gotten yet.  If less than -1, fstatat failed
-   with errno == ST->st_ino.  Otherwise, the status has already
-   been gotten, so return 0.  */
+/* Like fstatat, but cache on POSIX-compatible systems.  */
 static int
 cache_fstatat (int fd, char const *file, struct stat *st, int flag)
 {
-  if (st->st_size == -1 && fstatat (fd, file, st, flag) != 0)
+#if HAVE_STRUCT_STAT_ST_ATIM_TV_NSEC
+  /* If ST->st_atim.tv_nsec is -1, the status has not been gotten yet.
+     If less than -1, fstatat failed with errno == ST->st_ino.
+     Otherwise, the status has already been gotten, so return 0.  */
+  if (0 <= st->st_atim.tv_nsec)
+    return 0;
+  if (st->st_atim.tv_nsec == -1)
     {
-      st->st_size = -2;
+      if (fstatat (fd, file, st, flag) == 0)
+        return 0;
+      st->st_atim.tv_nsec = -2;
       st->st_ino = errno;
     }
-  if (0 <= st->st_size)
-    return 0;
-  errno = (int) st->st_ino;
+  errno = st->st_ino;
   return -1;
+#else
+  return fstatat (fd, file, st, flag);
+#endif
 }
 
 /* Initialize a fstatat cache *ST.  Return ST for convenience.  */
 static inline struct stat *
 cache_stat_init (struct stat *st)
 {
-  st->st_size = -1;
+#if HAVE_STRUCT_STAT_ST_ATIM_TV_NSEC
+  st->st_atim.tv_nsec = -1;
+#endif
   return st;
 }
 
-- 
2.34.1

From 03cc716cb1d6d69dfdb9038a6889035ab957f201 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 23 Jul 2022 11:00:33 -0700
Subject: [PATCH 2/2] ls: print negative file sizes as negative

This is more useful in practice (Bug#56710).
However, if POSIXLY_CORRECT is set, print them as positive.
* src/ls.c (abs_file_size, human_file_size): New functions.
(gobble_file, print_long_format): Use them.
* src/stat.c: Revert previous change, so that stat and ls agree.
---
 NEWS       |  6 ++++--
 src/ls.c   | 53 +++++++++++++++++++++++++++++++++++++++++++----------
 src/stat.c | 10 +---------
 3 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/NEWS b/NEWS
index 816025255..d76946eb8 100644
--- a/NEWS
+++ b/NEWS
@@ -21,12 +21,14 @@ GNU coreutils NEWS                                    -*- outline -*-
   'cp --reflink=always A B' no longer leaves behind a newly created
   empty file B merely because copy-on-write clones are not supported.
 
+  Unless POSIXLY_CORRECT is set, 'ls -l' no longer prints negative
+  file sizes as huge positive numbers.  This is more consistent with
+  how 'stat -c %s' treats virtual files like /proc/kcore.
+
   'ls -v' and 'sort -V' go back to sorting ".0" before ".A",
   reverting to the behavior in coreutils-9.0 and earlier.
   This behavior is now documented.
 
-  ’stat -c %s' now prints sizes as unsigned, consistent with 'ls'.
-
 ** New Features
 
   factor now accepts the --exponents (-h) option to print factors
diff --git a/src/ls.c b/src/ls.c
index d48892be7..475fb2719 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3142,6 +3142,19 @@ file_ignored (char const *name)
           || patterns_match (ignore_patterns, name));
 }
 
+/* The following functions assumes typical implementations
+   where off_t is no wider than uintmax_t.  */
+verify (OFF_T_MAX <= UINTMAX_MAX / 2);
+
+/* Return abs (SIZE) as an unsigned integer.  */
+
+static uintmax_t
+abs_file_size (off_t size)
+{
+  uintmax_t s = size;
+  return size < 0 ? -s : s;
+}
+
 /* POSIX requires that a file size be printed without a sign, even
    when negative.  Assume the typical case where negative sizes are
    actually positive values that have wrapped around.  */
@@ -3152,6 +3165,32 @@ unsigned_file_size (off_t size)
   return size + (size < 0) * ((uintmax_t) OFF_T_MAX - OFF_T_MIN + 1);
 }
 
+/* Store a string representation of the file size N into BUF,
+   which should have at LONGEST_HUMAN_READABLE + 2 bytes.
+   Return a pointer to the string.  */
+
+static char *
+human_file_size (off_t n, char *buf)
+{
+  if (n < 0)
+    {
+      static signed char posixly;
+      if (posixly == 0)
+        posixly = ls_mode == LS_LS && getenv ("POSIXLY_CORRECT") ? 1 : -1;
+      if (posixly < 0)
+        {
+          char *p = human_readable (abs_file_size (n), buf + 1,
+                                    file_human_output_opts, 1,
+                                    file_output_block_size);
+          *--p = '-';
+          return p;
+        }
+    }
+
+  return human_readable (unsigned_file_size (n), buf,
+                         file_human_output_opts, 1, file_output_block_size);
+}
+
 #ifdef HAVE_CAP
 /* Return true if NAME has a capability (see linux/capability.h) */
 static bool
@@ -3623,12 +3662,8 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
             }
           else
             {
-              char buf[LONGEST_HUMAN_READABLE + 1];
-              uintmax_t size = unsigned_file_size (f->stat.st_size);
-              int len = mbswidth (human_readable (size, buf,
-                                                  file_human_output_opts,
-                                                  1, file_output_block_size),
-                                  0);
+              char buf[LONGEST_HUMAN_READABLE + 2];
+              int len = mbswidth (human_file_size (f->stat.st_size, buf), 0);
               if (file_size_width < len)
                 file_size_width = len;
             }
@@ -4416,13 +4451,11 @@ print_long_format (const struct fileinfo *f)
     }
   else
     {
-      char hbuf[LONGEST_HUMAN_READABLE + 1];
+      char hbuf[LONGEST_HUMAN_READABLE + 2];
       char const *size =
         (! f->stat_ok
          ? "?"
-         : human_readable (unsigned_file_size (f->stat.st_size),
-                           hbuf, file_human_output_opts, 1,
-                           file_output_block_size));
+         : human_file_size (f->stat.st_size, hbuf));
       int pad;
       for (pad = file_size_width - mbswidth (size, 0); 0 < pad; pad--)
         *p++ = ' ';
diff --git a/src/stat.c b/src/stat.c
index 549762aba..3765a8f65 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -1492,14 +1492,6 @@ do_stat (char const *filename, char const *format,
 }
 #endif /* USE_STATX */
 
-/* POSIX requires 'ls' to print file sizes without a sign, even
-   when negative.  Be consistent with that.  */
-
-static uintmax_t
-unsigned_file_size (off_t size)
-{
-  return size + (size < 0) * ((uintmax_t) OFF_T_MAX - OFF_T_MIN + 1);
-}
 
 /* Print stat info.  Return zero upon success, nonzero upon failure.  */
 static bool
@@ -1583,7 +1575,7 @@ print_stat (char *pformat, size_t prefix_len, char mod, char m,
       fail |= out_mount_point (filename, pformat, prefix_len, statbuf);
       break;
     case 's':
-      out_uint (pformat, prefix_len, unsigned_file_size (statbuf->st_size));
+      out_int (pformat, prefix_len, statbuf->st_size);
       break;
     case 'r':
       if (mod == 'H')
-- 
2.34.1