Open SteveLauC opened 7 months ago
My current thought on this is that impl From<libc::xx> for Wrapper
can be unsafe
because we cannot ensure the values set in those libc types are valid
My current thought on this is that
impl From<libc::xx> for Wrapper
can beunsafe
because we cannot ensure the values set in those libc types are valid
Yes. Especially for cases where the libc type is a union, for example.
There are cases, where the data does not originate from C code, but from Rust code directly. Wouldn't it make sense to have a native Rust type that is guaranteed to be complete as alternative to the libc types? Something like:
/// A Rust representation of [libc::sockaddr_ll] with the additional
/// guarantee of valid data, so that this can be made into a [LinkAddr]
/// without the need of `unsafe` code.
///
/// # Examples
///
/// ```edition2021
/// use nix::sys::socket::{LinkAddr, SockAddrLl};
///
/// let addr = LinkAddr::from(SockAddrLl {
/// sll_family: libc::AF_PACKET as u16,
/// sll_ifindex: 0,
/// sll_protocol: 0,
/// sll_addr: [0; 8],
/// sll_halen: 0,
/// sll_hatype: 0,
/// sll_pkttype: 0,
/// });
/// ```
#[derive(Copy, Clone, Debug)]
pub struct SockAddrLl {
/// See [libc::sockaddr_ll::sll_family]
pub sll_family: u16,
/// See [libc::sockaddr_ll::sll_protocol]
pub sll_protocol: u16,
/// See [libc::sockaddr_ll::sll_ifindex]
pub sll_ifindex: i32,
/// See [libc::sockaddr_ll::sll_hatype]
pub sll_hatype: u16,
/// See [libc::sockaddr_ll::sll_pkttype]
pub sll_pkttype: u8,
/// See [libc::sockaddr_ll::sll_halen]
pub sll_halen: u8,
/// See [libc::sockaddr_ll::sll_addr]
pub sll_addr: [u8; 8]
}
impl From<SockAddrLl> for libc::sockaddr_ll {
fn from(sll: SockAddrLl) -> Self {
Self {
sll_family: sll.sll_family,
sll_protocol: sll.sll_protocol,
sll_ifindex: sll.sll_ifindex,
sll_hatype: sll.sll_hatype,
sll_pkttype: sll.sll_pkttype,
sll_halen: sll.sll_halen,
sll_addr: sll.sll_addr,
}
}
}
impl From<SockAddrLl> for LinkAddr {
fn from(sll: SockAddrLl) -> Self {
Self(sll.into())
}
}
There are cases, where the data does not originate from C code, but from Rust code directly. Wouldn't it make sense to have a native Rust type that is guaranteed to be complete as alternative to the libc types? Something like:
That involves doing data copies during every transition from C to Rust or vice versa. Nix strives for 0-cost abstractions instead. Wherever possible, Nix's Rust accessors should manipulate the raw C struct.
My suggestion does not replace any existing approach, but adds an alternative way for initialization that happens once for a socket. I'd argue that this optional one-time copy is a valid trade-off for getting rid of unsafe
code (if the copy doesn't get optimized away anyways). Adding #[inline]
to the two from()
functions, the compiler seems to actually eliminate the copying in the typical case, using opt level 1. I tried the following example on https://godbolt.org/:
pub struct LinkAddr(sockaddr_ll);
#[repr(C)]
pub struct sockaddr_ll {
pub sll_family: u16,
pub sll_protocol: u16,
pub sll_ifindex: i32,
pub sll_hatype: u16,
pub sll_pkttype: u8,
pub sll_halen: u8,
pub sll_addr: [u8; 8]
}
pub struct SockAddrLl {
pub sll_family: u16,
pub sll_protocol: u16,
pub sll_ifindex: i32,
pub sll_hatype: u16,
pub sll_pkttype: u8,
pub sll_halen: u8,
pub sll_addr: [u8; 8]
}
impl From<SockAddrLl> for sockaddr_ll {
#[inline]
fn from(sll: SockAddrLl) -> Self {
Self {
sll_family: sll.sll_family,
sll_protocol: sll.sll_protocol,
sll_ifindex: sll.sll_ifindex,
sll_hatype: sll.sll_hatype,
sll_pkttype: sll.sll_pkttype,
sll_halen: sll.sll_halen,
sll_addr: sll.sll_addr,
}
}
}
impl From<SockAddrLl> for LinkAddr {
#[inline]
fn from(sll: SockAddrLl) -> Self {
Self(sll.into())
}
}
pub fn usage(f: u16, p: u16, i: i32, h: u16, pk: u8, ha: u8, a: [u8; 8]) {
let addr = LinkAddr::from(SockAddrLl {
sll_family: f,
sll_ifindex: i,
sll_protocol: p,
sll_addr: a,
sll_halen: ha,
sll_hatype: h,
sll_pkttype: pk,
});
std::hint::black_box(&addr);
}
In order to get rid of the copy, we could also make SockAddrLl
a newtype around libc::sockaddr_ll
and let the user move all values into the constructor (hinting to and hoping for in-lining). Would you prefer that?
In order to get rid of the copy, we could also make
SockAddrLl
a newtype aroundlibc::sockaddr_ll
and let the user move all values into the constructor (hinting to and hoping for in-lining). Would you prefer that?
The constructor way has been discussed in this comment, and:
I don't like the proposed constructors because they're potentially too limited. On Linux and OSX, the libc structure has 7 fields. On the BSDs, it has between 8 and 10. We don't want to define a new constructor for every combination of fields that the user might want to initialize. I think it would be better to either use the Builder pattern, or else rely on getifaddrs to initialize the structure, and then provide mutators as necessary.
Related: #1977
As described in this comment, all the Nix wrapper types should be:
#[repr(transparent)]
impl From<libc::xx> for Wrapper
impl From<Wrapper> for libc::xx