-
Notifications
You must be signed in to change notification settings - Fork 654
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
New socket API #2199
New socket API #2199
Conversation
07316cd
to
8835561
Compare
Okay, so I have added a test that performs equality checks on the netmasks returned by I see two ways how to proceed from here:
I actually prefer the first option. To me, it seems to be the most robust and future proof choice, and we don't need to make assumptions anymore on how the system calls are behaving exactly. Edit: If we go with Option 1, then we'll likely need a new function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way too big to review. There are thousands of lines of changes here.
Some of them are purely formatting changes, which should never be mixed with
content changes. And specifically:
- The sendmsg changes should be broken out into a separate PR from the sockaddr changes.
- Your name changes don't match with existing precedent. For example, std::net:: always uses "Addr" instead of "Address". And within std::net, anything that combines an IP address with a port has "Sock" in the name. We should maintain that precedent. This PR's
Ipv4Address
type sounds like it is equivalent to std::net::Ipv4Addr instead of std::net::SocketAddrV4. - Why is enum the wrong abstraction for AddressFamily?
- Functions like
getsockname
now return a full sized sockaddr_storage unconditionally. That allocates a lot of extra memory that might not be needed. Previously, being generic, getsockname could be used with smaller sized types. - It seems to me that the main point of the PR is to replace
SockaddrLike
withAsRef<Addr>
. Could you please explain the motivation for that? - The
RawAddr
type only wrapslibc::sockaddr
. That doesn't even have enough room for asockaddr_in6
. That means thatRawAddr::to_ipv6
reads beyond the end of its reference. Instead, you should perform the cast from sockaddr to sockaddr_in6 while operating on raw pointers, before you turn anything into references. The fact that you had to add#[allow(unused)]
toRawAddr::new
is a clue that it's doing something wrong.
The current implementation has many issues. The goal of these changes is to not only fix these issues, but to make it harder to make similar mistakes again.
I agree, and I expected this response. I thought that for discussions it is easier to have everything in one PR, so you can see the whole picture. We can use this PR just for discussion and eventually create smaller, atomic PRs on the changes we agree on.
I just thought that I agree that the names could be misleading, as ip addresses live in layer 3, while the ports - coming from TCP or UDP - live in layer 4. Something that includes "Sock" would be more precise.
That's a tradeoff between performance and ergonomics. I also have an implementation that keeps the address type as a generic, but I ultimately opted for this approach as I valued ergonomics higher. In the end, it is your choice to make, I can live with both. I also took inspiration from socket2 which also just returns a wrapper around
With the The big advantage of In the current implementation, we instead have the
The safety documentation of the unsafe The And yes, this is the API I'm the most skeptical about. |
My motivation for the unsized address types was initially because of freebsd's Take a look at this test. A user unaware of this, could run into the following issue, thinking this is totally safe, but actually accesses memory that is out of bounds of let link_addr = getifaddrs(...).xzy();
let cloned = link_addr.clone();
let cloned_ptr = cloned.as_ptr();
unsafe {
println!("{}", *cloned_ptr.cast::<u8>().add(cloned.len() - 1));
} Of course, we can also just add another workaround for this, next to the macOS workaround. Hopefully this explains my motivations a bit better. |
Whether we want to use unsized types for addresses or not ultimately boils down to this "philosophical" question: Do we want it to be a bug, if functions like Or do we want it to be a missing feature? With my implementation, these things can be fixed by adding new functions to the types, like I think both approaches are valid and have their own strengths, and even I don't know which is better, or if my provided arguments are strong enough to justify these breaking changes. |
That makes sense. If users have a need for a custom value, then we should make it a struct.
A primary goal of Nix is zero-cost abstractions. We try to make the C APIs accessible to Rust but without imposing anything unnecessary. So let's keep these functions generic.
That won't happen, because it would break backwards-compatibility with every binary that uses the old sockaddr_un definition. And if Linux does introduce a sockaddr_un2, then in Nix we'll just have to exapand
Oh, ok. I thought it was because the
Well, it's obviously not "totally safe" since it's
I think yes. Also, FreeBSD documents sockaddr_dl's sdl_data lenght as "minimum work area, can be larger". So if we have any examples where it actually overflows, maybe we should create a larger container type of our own. |
Sounds good to me, I can agree on this. |
Still a soft WIP, but ready for a first review and open for suggestions. A changelog will be provided when we agreed on the changes.
This PR changes virtually everything related to socket.
Quick Overview:
AddressFamily
to a struct with associated constants as enums were the wrong abstraction here*Address
if sized or*Addr
if unsized)SockaddrStorage
got renamed toAddress
, and turned it into a struct.UnixAddress
,LinkAddress
andAddress
now have dyn-sized variants namedUnixAddr
,LinkAddr
andAddr
SockaddrLike
traitAsRef<Addr>
, which also replaces theSockaddrLike
boundRawAddress
, which can be used when the system call manages the address memory.getifaddrs
uses this new type.Incomplete list of issues and PRs affected by this PR:
Fixes #2177
Fixes #2176
Fixes #2054
Fixes #2030
Fixes #1990
Fixes #1939
Maybe Fixez #2031
Supersedes #2038
Supersedes #1992
Supersedes #1475
Checklist:
CONTRIBUTING.md