[Toybox] [PATCH] find.test: fix test expectation for BSD libcs.

[enh]

On Wed, Aug 28, 2019 at 4:19 PM Rob Landley <rob at landley.net> wrote:
>
> On 8/28/19 4:55 PM, enh wrote:
> > On Wed, Aug 28, 2019 at 2:44 PM Rob Landley <rob at landley.net> wrote:
> >>
> >> On 8/26/19 11:20 PM, enh wrote:
> >>> this comment had me worried:
> >>>
> >>> "Tweak DIRTREE_STATLESS so it returns zero stat for any error (I'm testing
> >>> that dev, ino, and blksize are all zero), and fill in file type from readdir()"
> >>>
> >>> and indeed, this breaks one of the find(1) tests i added. (and is how
> >>> we reopened this conversation about ls(1) --- the difference between
> >>> find which only ignores ENOENT and ls which ignores more.)
> >>
> >> Except the error I'm getting on my dir test isn't ENOENT, it's "permission
> >> denied" which is EACCES.
> >
> > that's ls, not find.
>
> It's common infrastructure, I'm trying to get it right for both.
>
> >> So which errors should it ignore? I changed it because only a couple things are
> >> using DIRTREE_STATLESS.
> >
> > but like i said, find and ls are demonstrably different in which errno
> > values they care about.
>
> Indeed, I was trying to catalog the behavior on both sides.
>
> >> Sigh, I don't have test cases for this.
> >
> > (there are some for find --- that's what your patch broke :-) )
>
> Yup. I've applied your patch but I'm not sure it's a good stopping point. (Today
> is just todo tangent city. Maybe because I'm tired.)
>
> >> You know, what it sounds like is DIRTREE_STATLESS needs to stick errno in one of
> >> the stat fields so the caller can check it when they care.
> >
> > my original suggestion was to add an extra field to struct dirtree for errno.
>
> I was thinking of using one of the other zeroed stat fields. We're already
> setting st_mode based on the readdir() info, sticking the errno in something
> like st_blocks isn't a big ask. (But it's also black magic and would need to be
> documented. I confirmed all the errnos are positive so we don't need to worry
> about signed/unsigned of any of those fields though.)
>
> > the patch i just sent out actually just reads errno directly, because
> > (right now at least) dirtree doesn't corrupt errno.
>
> Indeed. But that's kinda brittle and would at least need a comment in dirtree.c
> that things depend on it.
>
> > i also added another test for find, because the ENOENT special case
> > has a special case: paths provided on the command line are different
> > from discovered ones (which makes sense, because the former is likely
> > an error while the latter is likely just a dangling symlink, or
> > someone removing the same tree at the same time, in which case silence
> > isn't unreasonable).
>
> There's intentionally different codepaths for first call and recursive calls.
> That's why DIRTREE_STATLESS is set twice in find.c (once in the do_find() return
> code, and once in the argument to dirtree_flagread()).
>
> The original idea behind statless was the caller handles the errors,then you
> added the errno test in 99cfd03d8576, and I just took it back out again when
> fiddling with ls, and was just looking at find when I got your patch. :)
>
> >> I'm currently using
> >> st_dev, st_ino, and st_blksize are all zero (which should never happen;
> >> st_blksize should never be zero but I don't trust it and only one device/ino in
> >> the system can ever be 0/0 and I think it would be the first file in initramfs?)
> >>
> >> Ok, EACCES, ELOOP, ENOENT, ENAMETOOLONG, and sometimes ENOTDIR are _not_ pilot
> >> error. And the test you added was that ELOOP should NOT get caught. Hmmm...
> >>
> >> $ ls -l loop/
> >> ls: cannot access 'loop/': Too many levels of symbolic links
> >> $ ./toybox ls -l loop/
> >> -????????? ? ?    ?    ?                ? loop/
> >>
> >> It looks like ELOOP should always get caught as an error?
> >
> > i think you're confusing ls and find again.
>
> No, I'm trying to check all the "interesting" errors from stat in both contexts,
> not just fix the specific issue you raised. "They differ" means every case for
> every caller is now seperately interesting and has to be looked at. (And needs
> test cases the test suite.)
>
> >> $ find dirtest
> >> dirtest
> >> dirtest/three
> >> dirtest/dev
> >> dirtest/one
> >> dirtest/slink
> >> find: ‘dirtest/subdir’: Permission denied
> >> dirtest/subdir
> >> dirtest/two
> >> $ ./toybox find dirtest
> >> dirtest
> >> dirtest/three
> >> dirtest/dev
> >> dirtest/one
> >> dirtest/slink
> >> dirtest/two
> >>
> >> And that's a behavior difference too. (It complains _and_ shows it, mine's doing
> >> neither.)
> >
> > that _is_ a new one on me, but not one i'd seen in practice yet.
>
> I'm still not sure what the correct behavior is here, bur if you're happy with
> your patch I can throw:
>
> DIRTREE_STATLESS and errno:
>   http://lists.landley.net/pipermail/toybox-landley.net/2019-August/010837.html

if by "happy" you mean "can sync with upstream because the tests pass
again and stumble on to the next thing", which is about as good as we
can hope for in life, yes :-)

i do have a couple of other find irregularities to investigate, but
not related to this part of the code afaik. (i'm trying to switch AOSP
over.)

> into todo.txt and go back to poking at sh.c.
>
> Rob