nix icon indicating copy to clipboard operation
nix copied to clipboard

Switch to SockProtocolInt

Open Dev380 opened this issue 1 year ago • 4 comments

Fixes #1903 Currently, new/unknown protocols cannot be implemented with Unix sockets and this crate, because every protocol integer must be part of the SockProtocol enum. Additionally, some protocols, such as raw IPv4 with AF_PACKET, must hack around this by using an enum member with the same protocol number but entirely different name, leading to confusion.

This PR creates SockProtocolInt, which is a transparent struct wrapping an i32, and uses it in the library for everything. From<SockProtocol> is implemented so that no code doesn't break and it remains semver compatible.

I understand that the conditions specify using enums whenever possible for mutually exclusive options, however I believe that the lack of ergonomics afforded by this field specifically warrants a deviation.

Dev380 avatar Jul 07 '23 22:07 Dev380

I'm not sure why this errors on only a few systems

error: casting raw pointers to the same type and constness is unnecessary (`*const libc::sockaddr` -> `*const libc::sockaddr`)
    --> src/sys/socket/mod.rs:2261:17
     |
2261 |                 addr.assume_init().as_ptr() as *const sockaddr,
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `addr.assume_init().as_ptr()`
     |

AFAIK my changes haven't affected the socket address type.

Dev380 avatar Jul 07 '23 23:07 Dev380

This is a step backwards for ergonomics, by exposing the raw int values. If there's a specific protocol value that's not available now, then you should add it like how was done for CanBcm. Also, the Clippy warnings are fixed on the master branch.

asomers avatar Jul 15 '23 19:07 asomers

Would it be better to have something like this?

fn socket<T: IntoSockProtocol>(/*...*/ protocol: Option<T>) -> Result<RawFd>;

trait IntoSockProtocol {
    fn protocol(&self) -> c_int;
}

impl IntoSockProtocol for SockProtocol {
    fn protocol(&self) -> c_int { *self as _ }
}

impl IntoSockProtocol for c_int {
    fn protocol(&self) -> c_int { *self }
}

I'm not sure what you mean by "step backwards for ergonomics". The usage of socket doesn't change in almost all use cases (socket(..., Some(SockProtocol::Tcp))). The only ergonomics problem I see would be the documentation, where T: Into<Option<SockProtocolInt>> doesn't direct the user to the preferred SockProtocol. Making a PR for every possible protocol is also not an option for every user, because there could be custom protocols specific to some custom OSes.

nat-rix avatar Aug 10 '23 17:08 nat-rix

Yeah, actually, I do like that version better. What I don't want to do is to abandon type safety in the name of flexibility. However, instead of impl IntoSockProtocol for c_int, could you use a NewType instead? Just some syntactic salt to make the reader aware that the code is sending custom data to the OS, outside of the Rust type system. Something like struct CustomSockProtocol(pub c_int); impl IntoSockProtocol for CustomSockProtocol{...}.

asomers avatar Aug 11 '23 14:08 asomers