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

Prototype a new Buffer trait. #1290

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
28 changes: 28 additions & 0 deletions examples/new_read.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
use rustix::io::read;
use rustix::stdio::stdin;

fn main() {
let buf = Vec::new();
let _x: Vec<u8> = read(stdin(), buf).unwrap();

let mut buf = Vec::new();
let _x: usize = read(stdin(), &mut buf).unwrap();

let mut buf = [0, 0, 0];
let _x: usize = read(stdin(), &mut buf).unwrap();

// Why doesn't this work? This is reduced from src/fs/inotify.rs line 177.
struct Wrapper<'a>(&'a mut [u8]);
impl<'a> Wrapper<'a> {
fn read(&mut self) {
let _x: usize = read(stdin(), self.0).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is due to rust-lang/rust#35919. You need to reborrow manually: &mut *self.0.

One option is to make the Buffer trait impl owned types so the read method borrows it. Unfortunately I believe this requires rust 1.65. Also it seems kinda ugly so maybe not worth it.

pub fn read<Fd: AsFd, Buf: Buffer<u8> + ?Sized>(
    fd: Fd,
    buf: &mut Buf,
) -> io::Result<Buf::Result<'_>> {
    let len = backend::io::syscalls::read(fd.as_fd(), buf.as_maybe_uninitialized())?;
    // SAFETY: `read` works.
    unsafe { Ok(buf.finish(len)) }
}

pub trait Buffer<T> {
    /// The result of the process operation.
    type Result<'a>
    where
        T: 'a,
        Self: 'a;

    /// Convert this buffer into a maybe-unitiailized view.
    fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>];

    /// Convert a finished buffer pointer into its result.
    ///
    /// # Safety
    ///
    /// At least `len` bytes of the buffer must now be initialized.
    unsafe fn finish(&mut self, len: usize) -> Self::Result<'_>;
}

/// Implements [`Buffer`] around the a slice of bytes.
///
/// `Result` is a `usize` indicating how many bytes were written.
impl<T> Buffer<T> for [T] {
    type Result<'a>
        = usize
    where
        T: 'a;

    #[inline]
    fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
        // SAFETY: This just casts away the knowledge that the elements are
        // initialized.
        unsafe { core::mem::transmute::<&mut [T], &mut [MaybeUninit<T>]>(self) }
    }

    #[inline]
    unsafe fn finish(&mut self, len: usize) -> usize {
        len
    }
}

/// Implements [`Buffer`] around the a slice of uninitialized bytes.
///
/// `Result` is a pair of slices giving the initialized and uninitialized
/// subslices after the new data is written.
impl<T> Buffer<T> for [MaybeUninit<T>] {
    type Result<'a>
        = (&'a mut [T], &'a mut [MaybeUninit<T>])
    where
        T: 'a;

    #[inline]
    fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
        self
    }

    #[inline]
    unsafe fn finish(&mut self, len: usize) -> Self::Result<'_> {
        let (init, uninit) = self.split_at_mut(len);

        // SAFETY: The user asserts that the slice is now initialized.
        let init = slice::from_raw_parts_mut(init.as_mut_ptr().cast::<T>(), init.len());

        (init, uninit)
    }
}

/// Implements [`Buffer`] around the `Vec` type.
///
/// This implementation fills the buffer, overwriting any previous data, with
/// the new data data and sets the length.
#[cfg(feature = "alloc")]
impl<T> Buffer<T> for Vec<T> {
    type Result<'a>
        = usize
    where
        T: 'a;

    #[inline]
    fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
        self.clear();
        self.spare_capacity_mut()
    }

    #[inline]
    unsafe fn finish(&mut self, len: usize) -> usize {
        self.set_len(len);
        len
    }
}

}
}
let mut buf = Vec::new();
let mut wrapper = Wrapper(&mut buf);
wrapper.read();

// Why does this get two error messages?
let mut buf = [0, 0, 0];
let _x = read(stdin(), buf).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably because they're both valid. There's a hint assuming the type is an array and then the message that lists out all the valid types you can use.

}
11 changes: 9 additions & 2 deletions src/backend/libc/io/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use crate::io::ReadWriteFlags;
use crate::io::{self, FdFlags};
use crate::ioctl::{IoctlOutput, RawOpcode};
use core::cmp::min;
use core::mem::MaybeUninit;
#[cfg(all(feature = "fs", feature = "net"))]
use libc_errno::errno;
#[cfg(not(any(target_os = "espidf", target_os = "horizon")))]
Expand All @@ -26,8 +27,14 @@ use {
crate::io::{IoSlice, IoSliceMut},
};

pub(crate) unsafe fn read(fd: BorrowedFd<'_>, buf: *mut u8, len: usize) -> io::Result<usize> {
ret_usize(c::read(borrowed_fd(fd), buf.cast(), min(len, READ_LIMIT)))
pub(crate) fn read(fd: BorrowedFd<'_>, buf: &mut [MaybeUninit<u8>]) -> io::Result<usize> {
unsafe {
ret_usize(c::read(
borrowed_fd(fd),
buf.as_mut_ptr().cast(),
min(buf.len(), READ_LIMIT),
))
}
}

pub(crate) fn write(fd: BorrowedFd<'_>, buf: &[u8]) -> io::Result<usize> {
Expand Down
12 changes: 10 additions & 2 deletions src/backend/linux_raw/io/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,19 @@ use crate::ioctl::{IoctlOutput, RawOpcode};
#[cfg(all(feature = "fs", feature = "net"))]
use crate::net::{RecvFlags, SendFlags};
use core::cmp;
use core::mem::MaybeUninit;
use linux_raw_sys::general::{F_DUPFD_CLOEXEC, F_GETFD, F_SETFD};

#[inline]
pub(crate) unsafe fn read(fd: BorrowedFd<'_>, buf: *mut u8, len: usize) -> io::Result<usize> {
ret_usize(syscall!(__NR_read, fd, buf, pass_usize(len)))
pub(crate) fn read(fd: BorrowedFd<'_>, buf: &mut [MaybeUninit<u8>]) -> io::Result<usize> {
unsafe {
ret_usize(syscall!(
__NR_read,
fd,
buf.as_mut_ptr(),
pass_usize(buf.len())
))
}
}

#[inline]
Expand Down
119 changes: 119 additions & 0 deletions src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,128 @@

#![allow(unsafe_code)]

#[cfg(feature = "alloc")]
use alloc::vec::Vec;
use core::mem::MaybeUninit;
use core::slice;

/// A memory buffer that may be uninitialized.
pub trait Buffer<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how much it's worth discussing the implementation yet, but there are a few concerns:

  • I don't think this trait should be public (yet)
  • as_maybe_uninitialized is unsound in general, but not for our purposes. The conversion from u8 -> MaybeUninit<u8> is only valid if you never uninitialize the u8. We don't do that since we're always passing this data to a syscall, but for a public trait (and probably for ourselves just in case), the conversion to maybeuninit needs to be unsafe.
  • I don't think we should have an implementation for arrays ([T; N]). Maybe there's value I'm missing, but it seems simple enough to require you to pass in a slice instead.
  • Same argument for Vec<T>, you should be required to pass in &mut Vec<T> and it should use spare_capacity_mut (but not clear) plus set_len. Though maybe there's an argument for having the owned and borrowed versions with the owned one doing the clear.

/// The result of the process operation.
type Result;

/// Convert this buffer into a maybe-unitiailized view.
fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>];

/// Convert a finished buffer pointer into its result.
///
/// # Safety
///
/// At least `len` bytes of the buffer must now be initialized.
unsafe fn finish(self, len: usize) -> Self::Result;
}

/// Implements [`Buffer`] around the a slice of bytes.
///
/// `Result` is a `usize` indicating how many bytes were written.
impl<T> Buffer<T> for &mut [T] {
type Result = usize;

#[inline]
fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
// SAFETY: This just casts away the knowledge that the elements are
// initialized.
unsafe { core::mem::transmute::<&mut [T], &mut [MaybeUninit<T>]>(self) }
}

#[inline]
unsafe fn finish(self, len: usize) -> Self::Result {
len
}
}

/// Implements [`Buffer`] around the a slice of bytes.
///
/// `Result` is a `usize` indicating how many bytes were written.
impl<T, const N: usize> Buffer<T> for &mut [T; N] {
type Result = usize;

#[inline]
fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
// SAFETY: This just casts away the knowledge that the elements are
// initialized.
unsafe { core::mem::transmute::<&mut [T], &mut [MaybeUninit<T>]>(*self) }
}

#[inline]
unsafe fn finish(self, len: usize) -> Self::Result {
len
}
}

/// Implements [`Buffer`] around the a slice of bytes.
///
/// `Result` is a `usize` indicating how many bytes were written.
impl<T> Buffer<T> for &mut Vec<T> {
type Result = usize;

#[inline]
fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
// SAFETY: This just casts away the knowledge that the elements are
// initialized.
unsafe { core::mem::transmute::<&mut [T], &mut [MaybeUninit<T>]>(self) }
}

#[inline]
unsafe fn finish(self, len: usize) -> Self::Result {
len
}
}

/// Implements [`Buffer`] around the a slice of uninitialized bytes.
///
/// `Result` is a pair of slices giving the initialized and uninitialized
/// subslices after the new data is written.
impl<'a, T> Buffer<T> for &'a mut [MaybeUninit<T>] {
type Result = (&'a mut [T], &'a mut [MaybeUninit<T>]);

#[inline]
fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
self
}

#[inline]
unsafe fn finish(self, len: usize) -> Self::Result {
let (init, uninit) = self.split_at_mut(len);

// SAFETY: The user asserts that the slice is now initialized.
let init = slice::from_raw_parts_mut(init.as_mut_ptr().cast::<T>(), init.len());

(init, uninit)
}
}

/// Implements [`Buffer`] around the `Vec` type.
///
/// This implementation fills the buffer, overwriting any previous data, with
/// the new data data and sets the length.
#[cfg(feature = "alloc")]
impl<T> Buffer<T> for Vec<T> {
type Result = Vec<T>;

#[inline]
fn as_maybe_uninitialized(&mut self) -> &mut [MaybeUninit<T>] {
self.clear();
self.spare_capacity_mut()
}

#[inline]
unsafe fn finish(mut self, len: usize) -> Self::Result {
self.set_len(len);
self
}
}

/// Split an uninitialized byte slice into initialized and uninitialized parts.
///
/// # Safety
Expand Down
9 changes: 7 additions & 2 deletions src/fs/inotify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,16 @@
//! # }

#![allow(unused_qualifications)]
#![allow(dead_code)] // FIXME
#![allow(unused_imports)] // FIXME

use super::inotify;
pub use crate::backend::fs::inotify::{CreateFlags, ReadFlags, WatchFlags};
use crate::backend::fs::syscalls;
use crate::fd::{AsFd, OwnedFd};
use crate::ffi::CStr;
use crate::io;
use crate::io::{read_uninit, Errno};
use crate::io::{read, Errno};
use core::mem::{align_of, size_of, MaybeUninit};
use linux_raw_sys::general::inotify_event;

Expand Down Expand Up @@ -174,14 +176,17 @@ impl<'buf, Fd: AsFd> Reader<'buf, Fd> {
#[allow(clippy::should_implement_trait)]
pub fn next(&mut self) -> io::Result<InotifyEvent<'_>> {
if self.is_buffer_empty() {
match read_uninit(self.fd.as_fd(), self.buf).map(|(init, _)| init.len()) {
todo!("FIXME: see \"Why doesn't this work?\" in examples/new_read.rs");
/*
match read(self.fd.as_fd(), self.buf).map(|(init, _)| init.len()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

&mut *self.buf or the GAT version of the Buffer trait fixes this, as per my other comment.

Ok(0) => return Err(Errno::INVAL),
Ok(bytes_read) => {
self.initialized = bytes_read;
self.offset = 0;
}
Err(e) => return Err(e),
}
*/
}

let ptr = self.buf[self.offset..].as_ptr();
Expand Down
30 changes: 5 additions & 25 deletions src/io/read_write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#![allow(unsafe_code)]

use crate::buffer::split_init;
use crate::buffer::{split_init, Buffer};
use crate::{backend, io};
use backend::fd::AsFd;
use core::mem::MaybeUninit;
Expand All @@ -16,9 +16,6 @@ pub use backend::io::types::ReadWriteFlags;

/// `read(fd, buf)`—Reads from a stream.
///
/// This takes a `&mut [u8]` which Rust requires to contain initialized memory.
/// To use an uninitialized buffer, use [`read_uninit`].
///
/// # References
/// - [POSIX]
/// - [Linux]
Expand All @@ -40,27 +37,10 @@ pub use backend::io::types::ReadWriteFlags;
/// [illumos]: https://illumos.org/man/2/read
/// [glibc]: https://sourceware.org/glibc/manual/latest/html_node/I_002fO-Primitives.html#index-reading-from-a-file-descriptor
#[inline]
pub fn read<Fd: AsFd>(fd: Fd, buf: &mut [u8]) -> io::Result<usize> {
unsafe { backend::io::syscalls::read(fd.as_fd(), buf.as_mut_ptr(), buf.len()) }
}

/// `read(fd, buf)`—Reads from a stream.
///
/// This is equivalent to [`read`], except that it can read into uninitialized
/// memory. It returns the slice that was initialized by this function and the
/// slice that remains uninitialized.
#[inline]
pub fn read_uninit<Fd: AsFd>(
fd: Fd,
buf: &mut [MaybeUninit<u8>],
) -> io::Result<(&mut [u8], &mut [MaybeUninit<u8>])> {
// Get number of initialized bytes.
let length = unsafe {
backend::io::syscalls::read(fd.as_fd(), buf.as_mut_ptr().cast::<u8>(), buf.len())
};

// Split into the initialized and uninitialized portions.
Ok(unsafe { split_init(buf, length?) })
pub fn read<Fd: AsFd, Buf: Buffer<u8>>(fd: Fd, mut buf: Buf) -> io::Result<Buf::Result> {
let len = backend::io::syscalls::read(fd.as_fd(), buf.as_maybe_uninitialized())?;
// SAFETY: `read` works.
unsafe { Ok(buf.finish(len)) }
}

/// `write(fd, buf)`—Writes to a stream.
Expand Down
Loading