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

unistd: adding linux(glibc)/freebsd close_range. #2525

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ targets = [
]

[dependencies]
libc = { version = "0.2.160", features = ["extra_traits"] }
libc = { version = "0.2.162", features = ["extra_traits"] }
bitflags = "2.3.3"
cfg-if = "1.0"
pin-utils = { version = "0.1.0", optional = true }
Expand Down
1 change: 1 addition & 0 deletions changelog/2525.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add `close_range` in unistd for Linux/glibc and FreeBSD
55 changes: 55 additions & 0 deletions src/unistd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3992,3 +3992,58 @@ pub fn chflags<P: ?Sized + NixPath>(path: &P, flags: FileFlag) -> Result<()> {
Errno::result(res).map(drop)
}
}

#[cfg(any(
all(target_os = "linux", target_env = "gnu"),
target_os = "freebsd"
))]
#[cfg(feature = "fs")]
libc_bitflags! {
/// Options for close_range()
#[cfg_attr(docsrs, doc(cfg(feature = "fs")))]
pub struct CloseRangeFlags : c_uint {
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting, libc defines these 2 constants as unsigned integers. 🤔

#[cfg(all(target_os = "linux", target_env = "gnu"))]
/// Unshare the file descriptors range before closing them
Copy link
Member

Choose a reason for hiding this comment

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

Considering that the doc in the man page is kinda misleading, let's fix it on our side:

Suggested change
/// Unshare the file descriptors range before closing them
/// Unshare the file descriptor table, then close the file descriptors specified in the range.

CLOSE_RANGE_UNSHARE;
/// Set the close-on-exec flag on the file descriptors range
Copy link
Member

Choose a reason for hiding this comment

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

Let's be explicit:

Suggested change
/// Set the close-on-exec flag on the file descriptors range
/// Set the close-on-exec flag on the file descriptors range instead of closing them.

CLOSE_RANGE_CLOEXEC;
}
}

feature! {
#![feature = "fs"]

/// Close all the file descriptor from a given range.
/// An optional flag can be applied to modify its behavior.
#[cfg(any(
all(target_os = "linux", target_env = "gnu"),
target_os = "freebsd"
))]
pub fn close_range<F: std::os::fd::AsFd>(fdbegin: F, fdlast: F, flags: CloseRangeFlags) -> Result<Option<c_int>> {
Copy link
Member

Choose a reason for hiding this comment

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

I am curious why it returns a Result<Option<c_int>>, and the reason behind the code related to Errno. From the man page, it returns -1 on error, and 0 on success, looks like it is just a normal syscall👀

use std::os::fd::AsRawFd;

let raw = unsafe {
Errno::clear();

cfg_if! {
if #[cfg(all(target_os = "linux", target_env = "gnu"))] {
libc::syscall(libc::SYS_close_range, fdbegin.as_fd().as_raw_fd() as u32, fdlast.as_fd().as_raw_fd() as u32, flags.bits() as i32)
Copy link
Member

Choose a reason for hiding this comment

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

glibc added the wrapper in 2.34, and the libc crate has it exposed, any reason why we are using a raw syscall here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I call the wrapper but I think it is due to the CI when there is old glibc

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, some CIs are still using Ubuntu 20.04 due to this issue, which has glibc 2.31 😮‍💨, perhaps we can try bumping them to see if we are lucky now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can do that if we do not need to bother supporting older releases out in the field. I may come back to it later.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if we use glibc wrapper, then using this interface would require glibc 2.34 or above. We are still not lucky regarding that test: #2533, perhaps we should disable the test under QEMU then 😪

} else {
libc::close_range(fdbegin.as_fd().as_raw_fd() as u32, fdlast.as_fd().as_raw_fd() as u32, flags.bits() as i32)
}
}
};
if raw == -1 {
if Errno::last_raw() == 0 {
Ok(None)
} else {
Err(Errno::last())
}
} else {
#[cfg(all(target_os = "linux", target_env = "gnu", target_pointer_width = "64"))]
let raw = raw as i32;
Ok(Some(raw))
}

}
}
2 changes: 1 addition & 1 deletion test/sys/test_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2914,7 +2914,7 @@ mod linux_errqueue {
)
.unwrap();
// The sent message / destination associated with the error is returned:
assert_eq!(msg.bytes, MESSAGE_CONTENTS.as_bytes().len());
assert_eq!(msg.bytes, MESSAGE_CONTENTS.len());
// recvmsg(2): "The original destination address of the datagram that caused the error is
// supplied via msg_name;" however, this is not literally true. E.g., an earlier version
// of this test used 0.0.0.0 (::0) as the destination address, which was mutated into
Expand Down
2 changes: 1 addition & 1 deletion test/test_sendfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ fn test_sendfile_dragonfly() {
+ &trailer_strings.concat();

// Verify the message that was sent
assert_eq!(bytes_written as usize, expected_string.as_bytes().len());
assert_eq!(bytes_written as usize, expected_string.len());

let mut read_string = String::new();
let bytes_read = rd.read_to_string(&mut read_string).unwrap();
Expand Down
25 changes: 25 additions & 0 deletions test/test_unistd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1391,3 +1391,28 @@ fn test_group_from() {
assert_eq!(group.gid, group_id);
assert_eq!(group.name, "wheel");
}

#[test]
#[cfg(any(
all(target_os = "linux", target_env = "gnu"),
target_os = "freebsd"
))]
#[cfg_attr(qemu, ignore)]
fn test_close_range() {
use tempfile::NamedTempFile;
const CONTENTS: &[u8] = b"abcdef123456";
let mut tempfile1 = NamedTempFile::new().unwrap();
let mut tempfile2 = NamedTempFile::new().unwrap();
let mut tempfile3 = NamedTempFile::new().unwrap();
tempfile3.write_all(CONTENTS).unwrap();
tempfile2.write_all(CONTENTS).unwrap();
tempfile1.write_all(CONTENTS).unwrap();
let areclosed =
close_range(tempfile1, tempfile3, CloseRangeFlags::CLOSE_RANGE_CLOEXEC);
assert_eq!(
areclosed
.expect("close_range failed")
.expect("invalid flag"),
0
);
}