UDP: Store local and use local address in metadata

[chrysn]

Closes: #903

As discussed there, this is a breaking change, and no feature flag is set for it right now.

I'm surprised that things were this easy. So far, my only tests were whether this builds, thus leaving this as a draft PR. I expect to test this with code that would resolve embassy-rs/embassy#2516, at which point I'll mark it as ready.

[chrysn]

I'm having trouble finding the tests that need updating b/c they cover Debug output -- where do the test cases live? I failed to find anything matching case_3_ieee802154 in the sources.

[thvdveld]

We test the same test function for different cases by using a proc-macro. The name of the test is before ::case_something.

[thvdveld]

Tests are failing because the local_addr in the Meta is None when calling .into() on it, which is probably not what you want in the test cases.

[chrysn]

Tests are now updated and should pass.

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (5eced2a) 79.92% compared to head (dc60300) 79.93%.

Files	Patch %	Lines
src/socket/udp.rs	70.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #904      +/-   ##
==========================================
+ Coverage   79.92%   79.93%   +0.01%     
==========================================
  Files          80       80              
  Lines       28234    28255      +21     
==========================================
+ Hits        22566    22586      +20     
- Misses       5668     5669       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chrysn]

The coverage report rightfully complains lines in my change that are not covered. I've added a test that does set an explicit correct source address -- so the coverage should be better now.

We could have a second test on the topic of unadressability, but that first needs a decision: what if an application sends an invalid (eg. the unspecified) address as a source address? If we just let it pass, there are some protocol violating outcomes of this (like the unspecified address), and some questionable outcomes (if an IP address's lease time expired between accepting a request and sending a response, can I still send from that address?), but in practice that'd need either deliberate crafting of such a message, or keeping an address for overly long (and yes the application can't know what "overly long" is, but usually it will be fine).

Thus, my recommendation is that we accept whatever the application sets as a source address -- after all, smoltcp AIU doesn't limit applications' privileges, and the application could just send odd data through raw sockets. The stack may still start checking at a later point, and then consider the datagrams unaddressable. For tests, that means that while we could add a test like

diff --git a/src/socket/udp.rs b/src/socket/udp.rs
index c1313aa..d64ff18 100644
--- a/src/socket/udp.rs
+++ b/src/socket/udp.rs
@@ -740,6 +740,16 @@ mod test {
             ),
             Err(SendError::Unaddressable)
         );
+        assert_eq!(
+            socket.send_slice(
+                b"abcdef",
+                UdpMetadata {
+                    local_address: Some(IpvXAddress::UNSPECIFIED.into()),
+                    ..REMOTE_END.into()
+                }
+            ),
+            Ok(())
+        );
         assert_eq!(socket.send_slice(b"abcdef", REMOTE_END), Ok(()));
     }

it would just test the current behavior that is not guaranteed, and thus doesn't really add value.

[edit: Sorry for the force pushes, didn't have cargo fmt checks in my pre-commit hooks; thankfully git absorb came in handy cleaning that up in all the individual commits]

[chrysn]

I've now run this with some tests all the way to an embedded-nal-async application through embassy-rs/embassy#2519. Can't claim I've covered every edge case (that will come as I extend those applications), but I'm confident enough with the current state to mark it as ready for review.

[ivmarkov]

@Dirbaio @chrysn Is there anything outstanding in this PR prohibiting merging?

[chrysn]

From my PoV this is ready for final review -- especially if my recommendation of #904 (comment) is what we go with, but also if not (because even then the signatures would not change, and merely behavior of applications whose input violates the UDP/IP protocol may change between sending an erroneous packet vs. reporting an error).

[thvdveld]

Let's merge this!