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

cp: close file descriptors after cow on linux #2330

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

miDeb
Copy link
Contributor

@miDeb miDeb commented Jun 1, 2021

Instead of using into_raw_fd(), which transfers ownership and
requires us to close the file descriptor manually,
use as_raw_fd(), which does not transfer ownership to us but drops the
file descriptor when the original file is dropped (in our case at the
end of the function).

Closes #2312.
I tried to come up with a test for this, but I couldn't find a way to query the amount of file descriptors used, or to limit the number of open files for a test. If somebody could come up with a way to test this, that'd be awesome.

Instead of using into_raw_fd(), which transfers ownership and
requires us to close the file descriptor manually,
use as_raw_fd(), which does not transfer ownership to us but drops the
file descriptor when the original file is dropped (in our case at the
end of the function).
@miDeb miDeb marked this pull request as draft June 2, 2021 08:10
@miDeb miDeb marked this pull request as ready for review June 2, 2021 09:38
@SuperSandro2000
Copy link
Contributor

Return current file descriptor limit: ulimit -n
Set it: ulimit -n 10
get it of a process: cat /proc/<PID>/limits

@miDeb
Copy link
Contributor Author

miDeb commented Jun 2, 2021

I have also found ulimit, but how can I invoke it in a test (so that it sets the limit for the current test only)?

@miDeb
Copy link
Contributor Author

miDeb commented Jun 2, 2021

I think I have figured out a working solution (using rlimits), please check if it looks right...

@miDeb miDeb force-pushed the cp/close-fd branch 4 times, most recently from 709b76f to 8621b17 Compare June 2, 2021 16:36
@SuperSandro2000
Copy link
Contributor

I think I have figured out a working solution (using rlimits), please check if it looks right...

I don't know rust well enough to review that but I can say that it fixed the problem in NixOS when building ed with uutils-coreutils as gnu-coreutils replacement.

@@ -724,6 +729,8 @@ pub struct UCommand {
stdout: Option<Stdio>,
stderr: Option<Stdio>,
bytes_into_stdin: Option<Vec<u8>>,
#[cfg(target_os = "linux")]
limits: Vec<(rlimit::Resource, rlim, rlim)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

smart (adding it as part of the test framework)

@sylvestre sylvestre merged commit 5de623c into uutils:master Jun 3, 2021
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.

cp Too many open files (os error 24)
3 participants