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

initial commit readdir tweak #28

Merged
merged 2 commits into from Jun 7, 2023
Merged

Conversation

zedlopez
Copy link
Collaborator

@zedlopez zedlopez commented Jun 5, 2023

Skips entries for which readdir returns errors. Both rv and errno are zero when readdir has truly run out of files, so it shouldn't infinitely loop. Probably advisable to test on Mac OS X and have someone test on Windows before merging.

@zedlopez
Copy link
Collaborator Author

zedlopez commented Jun 5, 2023

addresses I7-2355

@zedlopez
Copy link
Collaborator Author

zedlopez commented Jun 6, 2023

...not that I'd ever want to discourage multiplatform testing, but given that this change is to POSIX Platforms.w, testing on Windows seems less crucial than I originally suggested.

@ganelson
Copy link
Owner

ganelson commented Jun 7, 2023

This change works absolutely fine for me on MacOS, but I am a bit nervous about the possibility of a hang if the error reported by readdir is more of the "this directory is such a mess that I can't read it at all" kind. The Gnu documentation for readdir (though not other sources) actually says:

"To distinguish between an end-of-directory condition or an error, you must set errno to zero before calling readdir. To avoid entering an infinite loop, you should stop reading from the directory after the first error."

I wonder if the loop should only be continued if errno is a particular value associated with an error on a bad single file or link?

@zedlopez
Copy link
Collaborator Author

zedlopez commented Jun 7, 2023

Yeah, I'd been having misgivings, too. I've committed a more discrete patch to my branch. OK, immediately after I commented that I didn't know why it hadn't updated the PR, I finally saw that it updated the PR.

@ganelson ganelson merged commit 6cf16f2 into ganelson:master Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants