Skip to content

io: implement [Read|Write]Volatile for &File and co #321

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 1, 2025
Merged

io: implement [Read|Write]Volatile for &File and co #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 coverage_config_x86_64.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"coverage_score": 92.92,
"coverage_score": 92.28,
"exclude_path": "mmap_windows.rs",
"crate_features": "backend-mmap,backend-atomic,backend-bitmap"
}
80 changes: 65 additions & 15 deletions src/io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::io::{Cursor, ErrorKind};
use std::io::Stdout;

#[cfg(feature = "rawfd")]
use std::os::fd::AsRawFd;
use std::os::fd::{AsFd, AsRawFd, BorrowedFd};

macro_rules! retry_eintr {
($io_call: expr) => {
Expand Down Expand Up @@ -127,7 +127,7 @@ pub trait WriteVolatile {
// We explicitly implement our traits for [`std::fs::File`] and [`std::os::unix::net::UnixStream`]
// instead of providing blanket implementation for [`AsRawFd`] due to trait coherence limitations: A
// blanket implementation would prevent us from providing implementations for `&mut [u8]` below, as
// "an upstream crate could implement AsRawFd for &mut [u8]`.
// "an upstream crate could implement AsRawFd for &mut [u8]".

macro_rules! impl_read_write_volatile_for_raw_fd {
($raw_fd_ty:ty) => {
Expand All @@ -137,7 +137,27 @@ macro_rules! impl_read_write_volatile_for_raw_fd {
&mut self,
buf: &mut VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
read_volatile_raw_fd(self, buf)
read_volatile_raw_fd(self.as_fd(), buf)
}
}

#[cfg(feature = "rawfd")]
impl ReadVolatile for &$raw_fd_ty {
fn read_volatile<B: BitmapSlice>(
&mut self,
buf: &mut VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
read_volatile_raw_fd(self.as_fd(), buf)
}
}

#[cfg(feature = "rawfd")]
impl ReadVolatile for &mut $raw_fd_ty {
fn read_volatile<B: BitmapSlice>(
&mut self,
buf: &mut VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
read_volatile_raw_fd(self.as_fd(), buf)
}
}

Expand All @@ -147,7 +167,27 @@ macro_rules! impl_read_write_volatile_for_raw_fd {
&mut self,
buf: &VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
write_volatile_raw_fd(self, buf)
write_volatile_raw_fd(self.as_fd(), buf)
}
}

#[cfg(feature = "rawfd")]
impl WriteVolatile for &$raw_fd_ty {
fn write_volatile<B: BitmapSlice>(
&mut self,
buf: &VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
write_volatile_raw_fd(self.as_fd(), buf)
}
}

#[cfg(feature = "rawfd")]
impl WriteVolatile for &mut $raw_fd_ty {
fn write_volatile<B: BitmapSlice>(
&mut self,
buf: &VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
write_volatile_raw_fd(self.as_fd(), buf)
}
}
};
Expand All @@ -159,7 +199,17 @@ impl WriteVolatile for Stdout {
&mut self,
buf: &VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
write_volatile_raw_fd(self, buf)
write_volatile_raw_fd(self.as_fd(), buf)
}
}

#[cfg(feature = "rawfd")]
impl WriteVolatile for &Stdout {
fn write_volatile<B: BitmapSlice>(
&mut self,
buf: &VolatileSlice<B>,
) -> Result<usize, VolatileMemoryError> {
write_volatile_raw_fd(self.as_fd(), buf)
}
}

Expand All @@ -174,18 +224,18 @@ impl_read_write_volatile_for_raw_fd!(std::os::fd::BorrowedFd<'_>);
///
/// Returns the numbers of bytes read.
#[cfg(feature = "rawfd")]
fn read_volatile_raw_fd<Fd: AsRawFd>(
raw_fd: &mut Fd,
Copy link
Member

@bonzini bonzini Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think &mut is more appropriate since you change the seek position into the file, or consume data from a pipe/socket.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, and I think that's why originally I didn't do this. But today I ended up checking the standard library, and they have impl Read for &File, e.g. advancing the seek position/consuming data through an immutable reference is possible for the stdlib version of these traits. I suppose this is considered okay because read-ing from a File doesn't change any Rust state (e.g. the File object itself remains unmodified), only kernel internal state.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I've changed the signature of this function to just take a RawFd instead of any kind of reference, but that doesn't really address your feedback, as it just moves the as_raw_fd() call up to the caller.

But since the standard library has impl Read for &File, do you still have any concerns about providing the same kind of impls here?

Copy link
Member

Choose a reason for hiding this comment

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

If we take a RawFd instead of a reference then we need to mark the function as unsafe because we cannot assume that the underlying fd is safe.

But since the standard library has impl Read for &File, do you still have any concerns about providing the same kind of impls here?

I think this is actually ok, but we either need to mark it as unsafe if we want to keep it as RawFd, or leave read_volatile_as_raw_fd as it was defined before,

Copy link
Member Author

Choose a reason for hiding this comment

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

If we take a RawFd instead of a reference then we need to mark the function as unsafe because we cannot assume that the underlying fd is safe.

argh, you're right. Even with the AsRawFd bound it would've required to be unsafe, because we cannot technically "trust" AsRawFd implementation. I've updated it to use those fancy new "io safety" types rust gained recently :)

fn read_volatile_raw_fd(
raw_fd: BorrowedFd<'_>,
buf: &mut VolatileSlice<impl BitmapSlice>,
) -> Result<usize, VolatileMemoryError> {
let fd = raw_fd.as_raw_fd();
let guard = buf.ptr_guard_mut();

let dst = guard.as_ptr().cast::<libc::c_void>();

// SAFETY: We got a valid file descriptor from `AsRawFd`. The memory pointed to by `dst` is
// valid for writes of length `buf.len() by the invariants upheld by the constructor
// of `VolatileSlice`.
// SAFETY: Rust's I/O safety invariants ensure that BorrowedFd contains a valid file descriptor`.
// The memory pointed to by `dst` is valid for writes of length `buf.len() by the invariants
// upheld by the constructor of `VolatileSlice`.
let bytes_read = unsafe { libc::read(fd, dst, buf.len()) };

if bytes_read < 0 {
Expand All @@ -205,18 +255,18 @@ fn read_volatile_raw_fd<Fd: AsRawFd>(
///
/// Returns the numbers of bytes written.
#[cfg(feature = "rawfd")]
fn write_volatile_raw_fd<Fd: AsRawFd>(
raw_fd: &mut Fd,
fn write_volatile_raw_fd(
raw_fd: BorrowedFd<'_>,
buf: &VolatileSlice<impl BitmapSlice>,
) -> Result<usize, VolatileMemoryError> {
let fd = raw_fd.as_raw_fd();
let guard = buf.ptr_guard();

let src = guard.as_ptr().cast::<libc::c_void>();

// SAFETY: We got a valid file descriptor from `AsRawFd`. The memory pointed to by `src` is
// valid for reads of length `buf.len() by the invariants upheld by the constructor
// of `VolatileSlice`.
// SAFETY: Rust's I/O safety invariants ensure that BorrowedFd contains a valid file descriptor`.
// The memory pointed to by `src` is valid for reads of length `buf.len() by the invariants
// upheld by the constructor of `VolatileSlice`.
let bytes_written = unsafe { libc::write(fd, src, buf.len()) };

if bytes_written < 0 {
Expand Down