Make all Nix libc wrapper types `repr(transparent)`/`From<libc::xx>`/`Into<libc::xx>`

[SteveLauC]

trafficstars

As described in this comment, all the Nix wrapper types should be:

1. #[repr(transparent)]
2. impl From<libc::xx> for Wrapper
3. impl From<Wrapper> for libc::xx

[SteveLauC]

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

[asomers]

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

Yes. Especially for cases where the libc type is a union, for example.

[jonatanzeidler]

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())
        }
    }

[asomers]

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.

[jonatanzeidler]

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);
}

[jonatanzeidler]

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?

[SteveLauC]

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?

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.

[SteveLauC]

Related: #1977