[Toybox] ls -l broken

[enh]

this was my original version, btw, which was a bit clearer. (i thought
you'd object to the repetition of `okay =`.)

it makes it more obvious that failing the first fstatat is not okay,
but if we both try and succeed the second fstatat, that makes up for
it:

    // stat dangling symlinks
    if (fstatat(fd, name, &st, sym)) {
      okay = 0;
      if (errno == ENOENT && !sym &&
              !fstatat(fd, name, &st, AT_SYMLINK_NOFOLLOW)) okay = 1;
    }

and then the statless++/goto error below happens whichever path you
take. (because remember that in the ls(1) case, errno == ENOENT and
sym != 0, so you can't succeed the second if in the current code.)

    if (!okay) {
      if (flags&DIRTREE_STATLESS) statless++;
      else goto error;
    }

On Wed, Nov 20, 2019 at 9:53 PM enh <enh at google.com> wrote:
>
> On Wed, Nov 20, 2019 at 9:52 PM enh <enh at google.com> wrote:
> >
> > On Wed, Nov 20, 2019 at 9:44 PM Rob Landley <rob at landley.net> wrote:
> > >
> > > 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...
> >
> > the point is that before the ls case where the first fstatat fails
> > couldn't fall through to the statless++/goto error block. so that
> > needed to move out, and then you need a way to tell how you got there.
> > (otherwise you _either_ fail the find tests _or_ you fail the [fixed]
> > ls tests.)
> >
> > > 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())
> >
> > no, the important part is moving the statless++/goto error part out
> > far enough that you can get to it whether or not you do a second
> > fstatat.
> >
> > the currently checked in code just carries on with an uninitialized struct stat.
> >
> > > 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.)
>
> (and i'm not sure what you mean by this. `export VERBOSE=1 ; make
> test_find && make test_ls` was working fine for me today. though
> you'll need to take my fix to the ls test to get any benefit from the
> latter half.)
>
> > > Rob