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

Add unistd::close_range() #2153

Closed
wants to merge 5 commits into from
Closed

Conversation

cpick
Copy link

@cpick cpick commented Oct 3, 2023

What does this PR do

This adds close_range(2) on Linux which allows efficiently closing (or marking "close on exec") many file descriptors at once without needing procfs.

I have added comments, but am unsure if new tests are warranted. I have manually tested and used test programs and strace to validate that it is working as intended (both with and without various CLOSE_RANGE_* flag permutations.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

This allows efficiently closing (or marking "close on exec") many file
descriptiors at once without needing procfs.
CHANGELOG.md Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member

Thanks for your contribution to nix!

I have left some comments :)


but am unsure if new tests are warranted

It would be great to test if this binding is properly working, we can test it with a single file descriptor with both first and last set to it

flags: CloseRangeFlags,
) -> Result<()> {
let res = unsafe {
libc::syscall(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be using raw syscalls here. They lack type safety, and they aren't portable. Instead, you should add close_range in a PR to libc. Then , once that merges, you can add support to Nix. If you're ambitious, you might consider adding closefrom while you're at it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I was following the precedent of other functions in this file (eg: gettid, execveat, and pivot_root).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For gettid() and execveat(), see this issue #2156, for pivot_root, there seems to be no libc wrapper for it, so we have to use the syscall directly.


close_range() has a wrapper in glibc (now sure about other libcs), can you please add it to the upstream libc crate and then we can add it in nix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense.

For my use case I need to statically link and are therefore using musl which does not yet implement close_range().

@SteveLauC
Copy link
Member

BTW, the CI failure is because an unsafe function needs a # Safety section, it can be added right above this doc:

/// Be aware that many Rust types implicitly close-on-drop, including
/// `std::fs::File`.  Explicitly closing them with this method too can result in
/// a double-close condition, which can cause confusing `EBADF` errors in
/// seemingly unrelated code.  Caveat programmer.

@cpick
Copy link
Author

cpick commented Oct 5, 2023

At this point it looks like I'd need to push the same syscall code down into libc and the resulting wrapper up here would still be (appropriately) unsafe.

This would leave very little benefit for my case for having the code in nix/libc and so I believe I'll just call syscall from my code and avoid another round of pull requests across the two crates.

Thank you both for taking the time to review this pull request, it was very pleasant to work with you for a moment and interact with the project.

@asomers
Copy link
Member

asomers commented Oct 5, 2023

Allright, @cpick . I'll close this PR for now. Feel free to reopen if you ever change your mind.

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

Successfully merging this pull request may close these issues.

3 participants