-
Notifications
You must be signed in to change notification settings - Fork 703
Description
Summary
Make using pread
et al nicer by getting rid of a special purpose struct and using a ubiquitous type instead.
Motivation
Using IoVec
in nix is a little cumbersome. At present, in nix, the IoVec
struct is defined:
pub struct IoVec<T> {
iov_base: *mut c_void,
iov_len: size_t,
phantom: PhantomData<T>
}
The C struct iovec
is defined:
struct iovec {
void *iov_base; /* Starting address */
size_t iov_len; /* Number of bytes */
};
This is pretty much equivalent to &[u8]
. See std::raw::Slice
:
pub struct Slice<T> {
pub data: *const T,
pub len: usize,
}
However, the T
type parameter is never used as anything other than [u8]
, and there is no way to even construct one with any other type.
Given that the C struct iovec
is effectively a slice, and that it exists because C lacks slices, I propose we use slices in its place for the Rust interface.
Examples
Signatures before:
pub fn writev(fd: RawFd, iov: &[IoVec<&[u8]>]) -> Result<usize>;
pub fn readv(fd: RawFd, iov: &mut [IoVec<&mut [u8]>]) -> Result<usize>;
After:
pub fn writev(fd: RawFd, iov: &[&[u8]]) -> Result<usize>;
pub fn readv(fd: RawFd, iov: &mut [&mut [u8]]) -> Result<usize>;
If you want to see what it would look like, I've got a partial replacement here: kamalmarhubi@44825ba
The modified tests pass.
Drawbacks
We're relying on a semi-coincidental layout equivalence. This makes me a bit uneasy.
Alternatives
- Leave it as is
- Drop the type parameter, making
IoVec
only store byte slices.
Activity
kamalmarhubi commentedon Feb 12, 2016
@arcnmx any thoughts here? This seems likke the kind of thing you'd have opinions on. :-)
arcnmx commentedon Feb 12, 2016
My thought is yes that makes for a better API, however, I'd be wary about whether the layout of slices is well-defined by the language or if we're just relying on implementation details.
#[repr(C)] struct IoVec<T> { v: *const T, s: usize }
is guaranteed to have the same memory layout as the Ciovec
struct. While slices are indeed represented as such, I'm not sure that the language guarantees this layout outright.So I like the proposed changes, but I'm not sure if they'd be relying on undefined or underspecified behaviour.
Changing it to
IoVec<'a>
sounds good to me if we're stuck with it and can't use regular slices.kamalmarhubi commentedon Feb 12, 2016
This makes sense. I'll try and bug some people on #rust-internals to see if my evil plan could be ok.
kamalmarhubi commentedon Feb 12, 2016
As a safety feature if we get CI on more platforms, we could have a test that creates a byte slice, transmutes to
libc::iovec
, and checks fields match. Not the same as guarantees though.raw
stabilization (raw::TraitObject
) rust-lang/rust#27751kamalmarhubi commentedon Mar 5, 2016
Link to the tracking issue for that type and the representation being stable: rust-lang/rust#27751
Also, I made a post on the users forum: https://users.rust-lang.org/t/how-can-i-feel-safe-in-doing-terribly-unsafe-things-like-relying-on-slice-layout/4857
kamalmarhubi commentedon Mar 10, 2016
The answers to the users forum post convinced me this would be a bad idea. Arguments against:
struct iovec
contains at least those fieldsThe only thing going for it is a cute convenience on some number of platforms.