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

UDP: Read and set local address on unbound sockets (IPV6_PKTINFO style functionality) #903

Closed
chrysn opened this issue Feb 2, 2024 · 4 comments · Fixed by #904
Closed

Comments

@chrysn
Copy link
Contributor

chrysn commented Feb 2, 2024

When UDP sockets are created and bound to a port but no IP address (the POSIX equivalent being being bound to [::]:${PORT}), applications often need to know which local address the packet was sent to, so that responses can be sent from the same address. This is requirement for implementing CoAP and (AIU) also QUIC -- and part of the embedded-nal-async (where more extensive rationale is given).

I can put in work on smoltcp to make this possible, but being new to the project I'll need a bit of guidance as to where to do that. My naive approach would be to add a field localendpoint to PacketMetadata, possibly behind a pktinfoish flag. Odd thing is that this is really not phy-level information -- it'd just as well fit in UdpMetadata, but that is not #[non_exhaustive] (should it be?).

For actually making it functional on the incoming side, I think I'd just need to update socket::udp::Socket::process to propagate data that is available in the ip_repr down into the metadata. On the send side, this would be in its dispatch(), where we'd have to decide whether we just take the application's word that the source address is good, or whether we check if it really matches self.endpoint.addr / is a valid source address of cx.

@Dirbaio
Copy link
Member

Dirbaio commented Feb 2, 2024

yes, UdpMetadata would be the correct place to add this.

@chrysn
Copy link
Contributor Author

chrysn commented Feb 2, 2024

So, I add it to UdpMetadata in a PR and worry about non_exhaustive and semver breakage later? Should I still make it opt-in, or make it unconditional? (Any implementation of embedded-nal-async on it, such as the one I'd plan for embassy, would enable the flag anyway, and it's about a 128bit copy and per-message storage that's added, maybe even saving on get_source_address calls in the outbound direction.)

@Dirbaio
Copy link
Member

Dirbaio commented Feb 2, 2024

don't worry about breakage, next release will likely be 0.12 (breaking bump) anyway.

I'm not sure about making it optional.. it's 16 extra bytes per packet, which is not much but also not insignificant. i'm sort of leaning towards not making it optional, because we already have enough feature flags 🥲

chrysn added a commit to chrysn-pull-requests/smoltcp that referenced this issue Feb 2, 2024
chrysn added a commit to chrysn-pull-requests/smoltcp that referenced this issue Feb 2, 2024
chrysn added a commit to chrysn-pull-requests/smoltcp that referenced this issue Feb 2, 2024
@chrysn
Copy link
Contributor Author

chrysn commented Apr 8, 2024

I've updated the PR as you requested -- there is now a local_address on UdpMetadata, and it is not optional. (Precisely: It is an Option<...> because a sender may not know where to send from / rely on the stack to set that, but not build-time configurable).

(Oups, I was looking at the issue and not the PR that has advanced -- but still, the PR is #904 is rather ready, so nonetheless nonetheless:)

We're still before the 0.12 release -- is this good to go in?

chrysn added a commit to chrysn-pull-requests/RIOT-rs that referenced this issue Apr 19, 2024
chrysn added a commit to chrysn-pull-requests/RIOT-rs that referenced this issue Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants