-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add musl and glibc bindings for getdents{,64} #4522
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
base: main
Are you sure you want to change the base?
Conversation
d6e29f6 to
5d4300a
Compare
|
I do not understand why freebsd tests would be failing if I have not modified any code or semver tests for freebsd. It appears the tests are failing on methods I have not modified (https://cirrus-ci.com/task/5573619788546048): |
a5a28a1 to
f16f1da
Compare
|
Ok, now I'm getting a failure in a musl shard ( The Any pointers on how one would typically debug this would be appreciated. I know the header |
|
Ok, after testing this locally against a separate project (since the |
|
Ok, it looks like the freebsd shards are failing on |
|
Sorry this took a while to get to. Could you explain why this is needed? The manpage says:
Which sounds deprecated |
|
For the followup needed, |
|
Reminder, once the PR becomes ready for a review, use |
|
Some changes occurred in OpenBSD module cc @semarie |
3ffae61 to
0ea4693
Compare
acb184c to
ee6b7ee
Compare
|
I specifically described an inability to run local testing with From reviewing recently merged PRs (https://github.com/rust-lang/libc/pulls?q=is%3Apr+is%3Aclosed), I can see that several changes were made to I see that the most recent commit to main also fails on the same freebsd-13 shard: d2ece10. So it appears that the tests are not helpful at all, and my change is passing all of the CI that everything else is. @tgross35 @semarie I have provided detailed links to manual pages. @tgross35, your first reply did not read the links I provided (according to the PR submission instructions) (along with a lurid degree of context), and instead you misinterpreted a link you searched on the web. There has since been no other responses. I see other changes such as #4729 with absolutely no information getting waved on through.
Please help me understand how to progress this in order to improve the performance of the rust stdlib. If there is something missing here, I am very willing to do more work, but right now I don't know how to proceed and the only feedback I have received did not even read the links I provided. A 2024 POSIX standard is the opposite of deprecated and this method has been available in the same stable ABI across all these platforms for at least a decade. I am also fine waiting, if it's necessary to block on CI fixes first. But this is blocking my further work on rustc and it's an exceptionally stable ABI across a wide variety of rust's supported platforms. @rustbot ready |
|
The Regarding how to make progress on the PR, I couldn't really help you. A friendly ping (like you did) is the way. Regarding #4729 you commented, the PR is part of a previous discussion I had with the OP (and I know him), I formally approved the PR as it touch only OpenBSD (and I agree that the PR description was very low and it is why I added some context in my comment). But I am not in position to merge it, so it will wait for someone else to merge it, as your current PR. Regards. |
|
Thanks so much for explaining that context. It is very reassuring to have this explanation and I appreciated your patience in describing how to understand these interactions. ^_^ |
|
Again, I'm sorry about the delay here. Note that reviews in this repo typically are in a 3-4 week cycle since I do them in batches, and currently I'm a couple weeks behind that so new API isn't the highest priority. There has been a pretty complete overhaul of testing and CI over the past few months, which likely explain some of what you've experienced. There's a lot to address here:
For reference, Rust's musl gets bumped occasionally. rust-lang/rust#142682 may have what you are interested in.
FreeBSD was failing for a while at this time due to issues with the old ctest, as you later noted. It's a non-required job though. FreeBSD 13 has a spurious segfault in Cargo (not libc-specific).
We do appreciate if anyone can test locally (especially for less common platforms), but there's no need to jump through hoops or bother with cross compiling.
Were there other problems besides MS_NOUSER?
Note that this PR just contains a lot more information than most, and has some weird interactions (e.g. glibc's documentation is pretty unusual and worth rectifying).
Rust's minimum glibc is 2.17 but getdents64 was added in 2.30 (per abilist), so std will still need to use the syscalls and isn't blocked on this for Linux. Anyway, I just have a few small requests then this can merge. Please squash as well. |
| extern "C" { | ||
| pub fn getdents(fd: c_int, buf: *mut crate::dirent, len: usize) -> c_int; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you combine this with the extern "C" block above?
| extern "C" { | ||
| // There is no glibc wrapper for the getdents system call, and getdents64() is only available | ||
| // from 2.30 onwards. | ||
| /// `buffer` points to a buffer of [`crate::dirent64`] structs. | ||
| pub fn getdents64(fd: c_int, buffer: *mut c_void, nbytes: usize) -> isize; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you merge this with the extern "C" block above?
Also no doc comments please, for this crate we defer to the platform manpages.
| @@ -0,0 +1 @@ | |||
| getdents64 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the file doesn't exist yet and Hurd is T3, it's not being checked. So this can just be dropped
|
|
||
| pub fn getdents(fd: c_int, buf: *mut c_char, nbytes: usize) -> c_int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one needs a link_name https://github.com/NetBSD/src/blob/bcc4c5ab138642408f293cdfc6eb84dbd7b7b1fb/include/dirent.h#L122
| extern "C" { | ||
| pub fn getdents(fd: c_int, buf: *mut c_char, nbytes: usize) -> isize; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this move into an existing extern "C" block?
Description
The method
posix_getdents()has recently been added in POSIX issue 8 (from 2024). This method offers a standard interface to retrieve multiple directory entries at once into a preallocated buffer. This typically translates directly to a syscall using the existingSYS_getdents{,64}key.However, as
posix_getdents()is so new, it has not been universally implemented yet. musl added it immediately last year, but glibc does not yet have it. Instead, glibc >= 2.30 has the methodgetdents64(), which is Linux-only.While musl appears to implement
posix_getdents()for all supported platforms, it appears that the version of musl rust pulls in is not from the local machine, but from its own bundled copy. This copy does not haveposix_getdents(), but instead simply hasgetdents64()to match glibc on linux, as well as agetdents()macro for compatibility with the BSD standard.The extant BSDs just name their version of this method
getdents(), without any64extension, and expose this as a libc method call instead of a syscall.Surprisingly, macOS does not support any form of the
getdents*()method. I mentioned this in passing to an Apple engineer a few weeks ago.I have been in contact with a glibc developer who plans to support this, but cannot make any timeline guarantees. Since it appears that the BSDs also have yet to add support for the POSIX standardized
posix_getdents()too, it does not seem terribly pressing to block this PR until upstream Rust pulls in a more recent version of musl. Instead, a followup PR should be able to triumphantly addposix_getdents()to all the POSIX platforms at once! ^_^ :DSources
posix_getdents()): https://github.com/bminor/musl/blob/86373b4999bfd9a9379bc4a3ca877b1c80a2a340/src/dirent/posix_getdents.c#L6getdents()): https://github.com/bminor/musl/blob/86373b4999bfd9a9379bc4a3ca877b1c80a2a340/src/linux/getdents.c#L6getdents64()): https://github.com/bminor/musl/blob/86373b4999bfd9a9379bc4a3ca877b1c80a2a340/include/dirent.h#L80getdents64()): https://github.com/bminor/glibc/blob/7eed691cc2b6c5dbb6066ee1251606a744c7f05c/sysdeps/unix/sysv/linux/bits/dirent_ext.h#L29getdents()): https://man.freebsd.org/cgi/man.cgi?query=getdents&sektion=2&manpath=freebsd-release-portsgetdents()): https://man.netbsd.org/getdents.2getdents()): https://man.openbsd.org/getdents.2getdents()): https://man.dragonflybsd.org/?command=getdents§ion=ANYChecklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated