Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to SockProtocolInt #2068

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Dev380
Copy link

@Dev380 Dev380 commented Jul 7, 2023

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 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
Copy link
Author

Dev380 commented Jul 7, 2023

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.

@asomers
Copy link
Member

asomers commented Jul 15, 2023

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.

@nat-rix
Copy link
Contributor

nat-rix commented Aug 10, 2023

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.

@asomers
Copy link
Member

asomers commented Aug 11, 2023

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{...}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SockProtocol: No support for redundant protocol values from different families
3 participants