[pal] Helper Function to Convert std::net::SocketAddrV4 to libc::sockaddr_in
#413
Conversation
src/rust/pal/linux/mod.rs
Outdated
| /// Converts a [std::net::SocketAddrV4] into a [libc::sockaddr_in]. | ||
| pub fn socketaddrv4_to_sockaddr_in(addr: &SocketAddrV4) -> libc::sockaddr_in { | ||
| libc::sockaddr_in { | ||
| sin_family: libc::AF_INET as u16, |
There was a problem hiding this comment.
I have no idea why libc defines AF_INET as a libc::c_int instead of as a libc::sa_family_t. If they had, you wouldn't need this "as" clause. But since you do need it, shouldn't it be "as libc::sa_family_t"?
There was a problem hiding this comment.
Thanks for noting this. u16 was also working, but I agree that moving to the correct type cast is better.
| sin_port: u16::to_be(addr.port()), | ||
| sin_addr: libc::in_addr { | ||
| s_addr: u32::to_be(u32::from_be_bytes(addr.ip().octets())), | ||
| }, |
There was a problem hiding this comment.
Again, libc uses its own type (libc::in_addr_t) as the type for s_addr. So, I think line 72 should technically have "as libc::in_addr_t" added to the end of it.
But yes, this conversion is correct. The version you're replacing was also correct (and avoided an unnecessary byte-swap), but only on little-endian machines. The ideal thing here would probably be to define our own pal::in_addr_t type, with a from_be_bytes() method that creates the equivalent of a big-endian 32-bit unsigned value.
There was a problem hiding this comment.
Leaving off the casting was working, but I introduced it, so that we are technically correct.
Also, I have introduced conditional compilation to select the right byte conversion technique.
| @@ -61,3 +62,15 @@ pub unsafe fn set_nonblock(fd: RawFd) -> i32 { | |||
| flags |= libc::O_NONBLOCK; | |||
| libc::fcntl(fd, libc::F_SETFL, flags, 1) | |||
| } | |||
|
|
|||
| /// Converts a [std::net::SocketAddrV4] into a [libc::sockaddr_in]. | |||
| pub fn socketaddrv4_to_sockaddr_in(addr: &SocketAddrV4) -> libc::sockaddr_in { | |||
There was a problem hiding this comment.
Ideally, this function wouldn't be here, it would be in src/rust/pal/sockets.rs (or somesuch), and it would return a value of a platform independent type, like pal::sockaddr_in (or somesuch). And then there would be separate implementations of it for linux and Windows.
I.e. all platform-dependent code would be under pal, and everything in catcollor, catnap, catpowder, et cetera would be platform-independent.
There was a problem hiding this comment.
I agree that we should discuss a better approach for structuring the PAL module. Let us have it and then I handle the necessary reorg.
This commit addresses #412
ppenna
force-pushed
the
enhancement-pal-sockaddr-conversion
branch
from
b2e0b6e
to
ca81f5d
Compare
Description
This PR closes #412
Summary of Changes
sockaddrv4_to_sockaddr_in()to perform the conversion between astd::net::SocketAddrV4tolibc::sockaddr_insin_portandsin_addr.s_addrto be on network order.