[Toybox] ls -l broken

[Rob Landley]

On 11/20/19 4:28 PM, enh via Toybox wrote:
> [PATCH] dirtree: fix 0b2cfcb8fdea9673f3c2e0940f1b16d5825e16ea.
> 
> 0b2cfcb8fdea9673f3c2e0940f1b16d5825e16ea broke `ls -l` on files that
> don't exist.
> 
> We didn't spot this because the corresponding ls test (which did exist)
> had accidentally been mangled in a couple of ways.

Sigh. THIS is why I hate testing for specific error messages. (I'm guessing
different' libc versions are giving different error messages, so this will break
again next year.)

> This patch fixes the test (and extends it to differentiate between a
> couple of possible ways it can go wrong), and fixes the broken path in
> dirtree_add_node.

I'm flying again today so got up early and brain's screwed up because of it, but
I don't understand why you moved this test:

+      sym = AT_SYMLINK_NOFOLLOW*!(flags&DIRTREE_SYMFOLLOW), okay = 1;

     // stat dangling symlinks
     if (fstatat(fd, name, &st, sym)) {
-      if (errno != ENOENT
-        || (!sym && fstatat(fd, name, &st, AT_SYMLINK_NOFOLLOW)))
-      {
-        if (flags&DIRTREE_STATLESS) statless++;
-        else goto error;
-      }
+      okay = (errno == ENOENT && !sym &&
+              !fstatat(fd, name, &st, AT_SYMLINK_NOFOLLOW));
+    }
+    if (!okay) {
+      if (flags&DIRTREE_STATLESS) statless++;
+      else goto error;
     }

If "okay" can only be zeroed when we go into the first fstatat()'s if() block,
what does moving the test to after the block accomplish? You don't use "okay"
again outside that stanza...

Let's see, old test:

  if (errno != ENOENT || (!sym && fstatat()))

was effectively replaced with:

  if (!(errno == ENOENT && !sym && !fstatat()))

But a straight inversion of the first test would be...

  if (!(errno == ENOENT && (sym || !fstatat())))

So it _looks_ like the fix you applied is basically just changing the existing
test to:

  if (errno != ENOENT || sym || fstatat())

The intent of "statless" being set was to return "exists but could not stat".
So we set it when the first stat fails, but the error isn't ENOENT and we
couldn't symYour change will do that when fstatat() fails with an error other
than ENOENT, if readdir returns something we can't stat without following symlinks.

Sigh. Why is ls defaulting to showing nanoseconds? How does this help? It makes
the output way wider and provides no useful information in most cases. (Debian's
ls -l doesn't even default to showing _seconds_...)

I'm too sleep deprived to think through this right now. (And I broke the
individual command builds on monday. Great.)

Rob