Skip to content

Comments

cp: close file descriptors after cow on linux#2330

Merged
sylvestre merged 4 commits intouutils:masterfrom
miDeb:cp/close-fd
Jun 3, 2021
Merged

cp: close file descriptors after cow on linux#2330
sylvestre merged 4 commits intouutils:masterfrom
miDeb:cp/close-fd

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.

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