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.

Sign up for GitHub

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

Jump to bottom

syscall: TestDirent failures with fewer than expected names #37323

Closed
bcmills opened this issue Feb 20, 2020 · 5 comments
Closed

syscall: TestDirent failures with fewer than expected names #37323

bcmills opened this issue Feb 20, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Linux release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.18

Comments

@bcmills
Copy link
Contributor

bcmills commented Feb 20, 2020

2020-02-19T20:58:16-f08734d/linux-arm
2020-02-04T23:15:01-e3f2e9a/linux-arm

--- FAIL: TestDirent (0.00s)
    dirent_test.go:35: tmpdir: /workdir/tmp/dirent-test550674696
    dirent_test.go:65: names: ["777777777777777777" "8888888888888888888" "99999999999999999999"]
    dirent_test.go:68: got 3 names; expected 10
FAIL
FAIL	syscall	0.353s

CC @randall77 @cherrymui @ianlancetaylor
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 20, 2020
@bcmills bcmills added this to the Backlog milestone Feb 20, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Jan 7, 2022

greplogs --dashboard -md -l -e 'FAIL: TestDirent .*\n(?: .+\n)* .*: got \d+ names; expected \d+' --since=2020-02-20

2022-01-07T02:37:20-40afced/linux-riscv64-jsing
2021-11-05T22:28:56-87a3fc5/linux-amd64-longtest
2021-10-02T13:15:42-808efd9-7bf7bbc/linux-riscv64-unmatched
2021-07-15T00:09:22-21a04e3/linux-riscv64-jsing
2021-05-07T01:43:57-30e306a-07d8cba/linux-arm-scaleway
2021-04-02T20:26:12-35a8bbc/linux-arm-scaleway
2021-03-11T20:01:00-4dd9c7c/linux-arm

@bcmills bcmills changed the title syscall: TestDirent failures on linux-arm builder syscall: TestDirent failures with fewer than expected names Jan 7, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Jan 7, 2022

I notice that the test has a seemingly-arbitrary direntBufSize constant that lacks any comment as to its derivation:
https://cs.opensource.google/go/go/+/master:src/syscall/dirent_test.go;l=25;drc=f229e7031a6efb2f23241b5da000c3b3203081d6

If I reduce that constant to 128, I get exactly this failure mode. So I wonder if perhaps the constant is incorrect on some systems under some conditions.

The “blame” layer shows that the constant comes from CL 141297 (CC @paulzhol @bradfitz @ianlancetaylor).

@bcmills bcmills modified the milestones: Backlog, Go1.18 Jan 7, 2022
@bcmills bcmills added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jan 7, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Jan 7, 2022

This looks like an easily-fixed bug in the test, and it has failed on linux/arm and linux/amd64 which are both first-class ports. Marking as release-blocker, and I will mail a fix.

@bcmills bcmills self-assigned this Jan 7, 2022
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/376334 mentions this issue: syscall: in TestDirent, make as many ReadDirent calls as are needed

@paulzhol
Copy link
Member

paulzhol commented Jan 7, 2022 •
edited
Loading

IIRC that test originated from a freebsd-only fix for parsing direntries retuned from the cooked syscall.

I notice that the test has a seemingly-arbitrary direntBufSize constant that lacks any comment as to its derivation..

I think direntBufSize relates to a constant called DIRBLKSIZ which is set to 1024 bytes on all BSDs. It is the minimum size supported for that familiy of syscalls. So 2048 was chosen to be big enought.
But I also think 128 seems too low, probably on Linux as well. There is even a bug related to a too-small buffer size: #24015 on CIFS in Linux.

All the size calculations in that test with multiples of 4 or 8 are due to the freebsd 11/12 inode64 compatability shim which can now be removed, now that freebsd 11 is no longer supported.

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
@jproberts
syscall: in TestDirent, make as many ReadDirent calls as are needed
85e250f 
ReadDirent returns only as many directory entries as will fit in the
buffer, and each entry is variable-length — so we have no guarantee in
general that a buffer of a given arbitrary size can hold even one
entry, let alone all ten entries expected by the test.

Instead, iterate calls to ReadDirent until one of the calls returns
zero entries and no error, indicating that the directory has been read
to completion.

Fixes golang#37323

Change-Id: I7f1cedde7666107256604e4ea1ac13c71f22151a
Reviewed-on: https://go-review.googlesource.com/c/go/+/376334
Trust: Bryan Mills <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@rsc rsc unassigned bcmills Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Linux release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@paulzhol @bcmills @gopherbot