Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing/fstest: test that ReadDir on file returns error, or returns 0 entries if file is empty #44967

Open
bcmills opened this issue Mar 12, 2021 · 17 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 12, 2021

The Plan 9 builders are consistently failing on the new check added to testing/fstest.TestFS in CL 296391

--- FAIL: TestDirFS (0.07s)
    os_test.go:2720: TestFS found errors:
        a: ReadDir of non-dir file should return an error
        b: ReadDir of non-dir file should return an error
        dir/x: ReadDir of non-dir file should return an error
FAIL
FAIL	os	9.258s

2021-03-12T02:56:04-e8b8278/plan9-arm
2021-03-12T02:30:33-e87c4bb/plan9-arm
2021-03-12T01:47:01-a607408/plan9-arm
2021-03-12T00:52:00-71a6c13/plan9-arm
2021-03-11T21:43:04-7fc638d/plan9-arm
2021-03-11T20:46:21-b3896fc/plan9-arm
2021-03-11T20:01:00-4dd9c7c/plan9-arm
2021-03-11T19:11:46-43d5f21/plan9-arm
2021-03-11T19:04:42-b0733ba/plan9-386-0intro
2021-03-11T19:04:42-b0733ba/plan9-arm
2021-03-11T19:00:06-64d323f/plan9-arm
2021-03-11T18:50:12-ae9cd12/plan9-arm
2021-03-11T18:25:50-86d6678/plan9-386-0intro
2021-03-11T18:25:50-86d6678/plan9-arm
2021-03-11T18:25:41-1853411/plan9-arm

It's not obvious to me whether this is a defect in the os implementation of ReadDir on Plan 9, or an overly-optimistic assertion in testing/fstest.TestFS.

CC @josharian @rsc @ianlancetaylor @fhs @millerresearch @0intro

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 12, 2021

Marking as release-blocker for Go 1.17 until we determine whether this is a defect in testing/fstest (which would affect all platforms), or in ReadDir on plan9 (which would not necessarily need to block the release because plan9 is not a first-class port).

Loading

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 12, 2021

(And if this is a defect on plan9, the test still ought to be fixed or bypassed somehow so that the builders can provide useful test coverage for everything else.)

Loading

@bcmills bcmills added the Soon label Mar 12, 2021
@gopherbot gopherbot closed this in 9289c12 Mar 12, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 12, 2021

Change https://golang.org/cl/301190 mentions this issue: Revert "testing/fstest: test that ReadDirFile on a non-dir fails"

Loading

@fhs
Copy link
Contributor

@fhs fhs commented Mar 12, 2021

It seems to be a defect in the os implementation of ReadDir on Plan 9. ReadDir succeeds on empty files:

cpu% cd src/os
cpu% /tmp/readdir testdata/hello
2021/03/12 12:37:56 readdir testdata/hello: stat buffer too short
cpu% /tmp/readdir testdata/dirfs/a
cpu% /tmp/readdir testdata/dirfs/b
cpu% /tmp/readdir testdata/dirfs/dir/x
cpu% /tmp/readdir /lib/words
2021/03/12 12:39:39 readdir /lib/words: malformed stat buffer

Loading

@millerresearch
Copy link
Contributor

@millerresearch millerresearch commented Mar 12, 2021

On Plan 9, the same read syscall is used to read both directories and regular files. There's a higher level dirread library function to read raw directory entries and parse them into Dir structures, but there's no prohibition against using that function to read from a regular file which happens to contain zero or more syntactically correct directory entries. Go's ReadDir is semantically very close to Plan 9's dirread, but go doc for the fs.ReadDirFile interface says:

... ReadDir should return an error for non-directories.

Does should in Go documentation have the same meaning as in Internet RFCs, or does it actually mean must? And if so, should (or must) the same restriction apply to os.ReadDir and os.File.ReadDir and os.Readdir and os.File.Readdir and os.File.Readdirnames? (Any others I've missed?)

Loading

@fhs
Copy link
Contributor

@fhs fhs commented Mar 13, 2021

For reference, APE's opendir does a stat at the beginning to check if it's a directory. In 9front, they changed it by removing the stat call and instead added a slash to the end of filename. Open syscall returns an error if there is a slash after a regular filename:

cpu% syscall open /lib/words 0
syscall: return: 3
cpu% syscall open /lib/words/ 0
syscall: return: -1 error: '/lib/words/' not a directory

Too bad we're dealing with a file that's already open, so we can't use this trick to avoid the stat syscall. So, depending on the meaning of "should", we may need to add an extra stat syscall in readdir, possibly only when we read nothing from the file to check whether it's an empty regular file or empty directory.

Loading

@josharian
Copy link
Contributor

@josharian josharian commented Mar 13, 2021

We had a similar conversation about FreeBSD and the cost of an extra stat at https://groups.google.com/g/golang-dev/c/rh8jwxyG1PQ.

I personally am tempted to pay the cost of the stat in each case in order to have stronger fs.FS invariants.

This and the FreeBSD case seem like a policy decision, and they should probably be decided together.

I'm going to re-open this as a place to make that decision.

cc @rsc who wrote the original "should" in the docs

Loading

@josharian josharian reopened this Mar 13, 2021
@millerresearch
Copy link
Contributor

@millerresearch millerresearch commented Mar 13, 2021

In practice, applications doing a ReadDir will often have previously done a stat to check whether the file is a directory. File.Stat could cache the file type, to save the extra stat syscall in case it's followed by ReadDir. If the application uses os.Stat instead or doesn't check, ReadDir can do a stat syscall and cache the file type for subsequent calls.

Loading

@toothrot
Copy link
Contributor

@toothrot toothrot commented Mar 25, 2021

@rsc Do you think an extra stat call is worth it, per @josharian and @millerresearch?

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Apr 8, 2021

@rsc Can you please take a look since this is a release blocking issue in NeedsDecision state?

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 19, 2021

To clarify this issue, the test is no longer failing. It was fixed by https://golang.org/cl/301190.

The question here is whether an implementation of io/fs.FS is required to have ReadDir fail for a file that is not a directory.

Loading

@ianlancetaylor ianlancetaylor changed the title os,testing/fstest: TestDirFS failing on plan9 since CL 296391 io/fs: should ReadDir always fail for a file that is not a directory? Apr 22, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 29, 2021

Reading a directory, as succeeds on FreeBSD, was standard Unix behavior for a very long time. (You read the directory bytes and parse the entries out.) That clearly should be allowed.

This issue is the opposite: ReadDir'ing a non-directory. Of course, if you can read a file and you read a directory by reading it like a file, the situation is going to get muddied. But I'm still confused how ReadDir of a non-directory file can be succeeding on Plan 9, since very few files would be found to contain well-formatted directory entries. I would expect at least a parse error. Is ReadDir only succeeding for empty files, which would be misinterpreted as empty directories?

The Plan 9 kernel knows for any open fd whether it is connected to a directory or not, because that's one of the bits that comes back in the Ropen message. But there is no existing syscall that makes that info easily available to user space.

Perhaps it would be enough to test that you get zero directory entries, error or not?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Apr 29, 2021

Given that the test is no longer failing I don't believe the release is blocked waiting for additional work in this package.

Loading

@millerresearch
Copy link
Contributor

@millerresearch millerresearch commented Apr 29, 2021

But I'm still confused how ReadDir of a non-directory file can be succeeding on Plan 9, since very few files would be found to contain well-formatted directory entries. I would expect at least a parse error. Is ReadDir only succeeding for empty files, which would be misinterpreted as empty directories?

Yes, that's exactly the case which was causing the test failure: ReadDir was reading an empty file, and not returning the expected error (because an empty file is a correctly formatted directory of zero entries).

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Apr 29, 2021

Thanks. I think it's fine for an empty file to ReadDir successfully on Plan 9 (and presumably FreeBSD and other directory-reading operating systems).

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 29, 2021

Thanks. There is nothing that must change here, so removing the release-blocker label and moving to backlog. Someone can update testing/fstest to test for this behavior, but there is no urgency.

Loading

@ianlancetaylor ianlancetaylor removed this from the Go1.17 milestone Apr 29, 2021
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Apr 29, 2021
@ianlancetaylor ianlancetaylor changed the title io/fs: should ReadDir always fail for a file that is not a directory? testing/fstest: test that ReadDir on file returns error, or returns 0 entries if file is empty Apr 29, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 5, 2021

Change https://golang.org/cl/332869 mentions this issue: testing/fstest: test that ReadDirFile on a non-dir fails

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants