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

SocketAddr associated types #92

Open
chrysn opened this issue Oct 22, 2023 · 9 comments
Open

SocketAddr associated types #92

chrysn opened this issue Oct 22, 2023 · 9 comments

Comments

@chrysn
Copy link
Contributor

chrysn commented Oct 22, 2023

Implementing embeded-hal(-async), it seems I'm always converting SocketAddr back and forth with the platform's own socket addr type.

Would it make sense to add an associated SocketAddr type (probably constrained to Into<SocketAddr> + TryFrom<SocketAddr, Error=Unavailable> + PartialEq<Self::SocketAddr> + Hash)? In all places where we previously pass SocketAddr, we'd now pass Self::SocketAddr. Wherever users store addresses, they'd use their stack's SocketAddr type, and only convert through a SocketAddr when entering or leaving the stack (or even not at all if we require Display and FromStr).

Some reasons why a stack would not like working with SocketAddr all the time:

  • It is built on something where Rust's SocketAddr is just not the native format (eg. any non-Rust network stacks), so it needs to copy fields around all the time. (This is the case with RIOT OS)
  • It is an IPv6 only stack that doesn't constantly want to deal with erring out on IPv4 addresses.
  • It is an IPv4/6 dual stack that internally represents IPv4 addresses as IPv4-mapped addresses.
  • It has only a single network interface and thus doesn't keep a zone identifier around. (In RIOT, there are some optimizations for what is called "highlander mode" when there can be only one).

This is mainly a concern for users of unconnected UDP sockets, where recvfrom-style use is predominant, and addresses are handled in every single packet. For TCP or connected UDP sockets, it's probably a secondary concern.

@ryan-summers
Copy link
Member

I am very okay with a generic socketaddr type myself, as I've had these exact same issues during implementation. I don't see any specific benefit to using no-std-net, since we still have versioning issues, and there's not very convenient methods to convert between common types.

@ryan-summers
Copy link
Member

However, how would a library that is agnostic to the underlying stack know how to construct and use these addresses? I.e. https://github.com/quartiq/minimq/blob/master/src/network_manager.rs, where we don't know what stack we're running on. Would this be the TryFrom impl bound?

@chrysn
Copy link
Contributor Author

chrysn commented Nov 15, 2023

The versioning issues probably go away as we might use the opportunity to switch to core::net::SocketAddr as a reference type. Indeed I'd think that TryFrom<core::net::SocketAddr> and Into<core::net::SocketAddr> (Or From<Self> for SocketAddr, I always mix up which is the good one to implement) should be part of the trait bounds -- in particular, core::net::SocketAddr would stay a perfectly valid choice there.

In code such as the network managers case, there's the TcpStack: TcpClientStack generic type around, so options are (as a matter of taste, partially) to either put TcpStack::SocketAddr in the own object's signtures, or to put SocketAddr there and go through the conversions (or to go half way and put impl TryInto<TcpStack::SocketAddr> in there).

@ryan-summers
Copy link
Member

I would love to use core::net, but it's still nightly-only behind feature(ip_in_core)

@chrysn
Copy link
Contributor Author

chrysn commented Nov 15, 2023

Oh -- indeed, sorry, I didn't check all the way up on the documentation tree (and the types don't get the warning if their module is gated). In that case, the proposed associated types are not helping the versioning situation (but don't don't make it worse either, for where we used to use no-std-net's SocketAddr we'd now use TryFrom/Into it).

@chrysn
Copy link
Contributor Author

chrysn commented Feb 12, 2024

The trouble of core::net stabilization is just about to go away, and #102 already does the breaking API change. As there is renewed interest in this from #106 (CC'ing @ivmarkov), I propose we give this a try.

Processing input from #106, it may make sense (but may need discussion) to

  • make the address types short-lived (because they may be pointers into the socket)
  • use a single type for a (local sockaddr, remote sockaddr) type, which would then not have the TryFrom/Into bounds on the AT, but rather be a trait that has a .local() -> core::net::SockAddr, .remote() -> ... accessor and a from(local, remote) -> Result<Self, ...> constructor. Nice aspects of doing this are that mismatches are caught early (no you can't send to an IPv6 address from an IPv4 bound socket), and that connected sockets can have a single auxiliary item in their signatures.

@ryan-summers
Copy link
Member

Closing this as resolved by #102, as we now use the freshly-stabilized core::net :)

If my assessment is wrong here, feel free to reopen!

@chrysn
Copy link
Contributor Author

chrysn commented Mar 22, 2024

Please reopen (I don't have the permssions to); 102 does not fix this: the core socket addresses are a more-easily-to-agree-on type, but socket implementations may still have a local better type.

The easiest example to give is an IPv4-only stack that has thus has a smaller socket address type (yikes, but it is the simplest example); a better example are 6LoWPAN implementations that only communicate locally and thus use compressed addresses, or where addresses IP addresses are thin pointers into refcounted neighbor table entries.

@chrysn
Copy link
Contributor Author

chrysn commented Mar 22, 2024

The core transition was mainly mentioned here because it'd be a breaking change anyway, and would thus be an opportunity to introduce the associated types (where indeed many stacks would then put core::net::SockAddr as the AT, because it is TryFrom and Into itself).

@eldruin eldruin reopened this Mar 22, 2024
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

No branches or pull requests

3 participants