io: implement [Read|Write]Volatile for &File and co #321
Conversation
| @@ -175,7 +205,7 @@ 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, | |||
There was a problem hiding this comment.
I think &mut is more appropriate since you change the seek position into the file, or consume data from a pipe/socket.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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 :)
src/io.rs
Outdated
| @@ -206,7 +236,7 @@ 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, | |||
| raw_fd: &Fd, | |||
roypat
force-pushed
the
read-write-by-ref
branch
from
01e9b7d to
1e36e8a
Compare
| @@ -175,7 +205,7 @@ 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, | |||
There was a problem hiding this comment.
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,
Implement these traits for references to types that implement them, e.g. &File, &TcpStream, &UnixStream, &Stdout, etc. This mirrors the standard library implementations and allows calling read/write methods from contexts where no mutable reference is available. Signed-off-by: Patrick Roy <[email protected]>
Implement these traits for references to types that implement them, e.g. &File, &TcpStream, &UnixStream, &Stdout, etc. This mirrors the standard library implementations and allows calling read/write methods from contexts where no mutable reference is available.
There's probably something more clever we can do with the traits to avoid some repetition, but really, isn't not that bad imo.
Summary of the PR
Please summarize here why the changes in this PR are needed.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.