On Wed, Nov 20, 2019 at 9:44 PM Rob Landley <[email protected]> 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.) > > Rob _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
