search

smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.64k stars 404 forks source link

refactor: move IPv6 HBH in own module #827

Closed thvdveld closed 11 months ago

thvdveld commented 11 months ago

The Ipv6HopByHopRepr was previously the same as an Ipv6ExtHeader. However, the RFC says that hop-by-hop options might update en route, which is not possible with the current implementation.

I added a representation for the Hop-by-Hop header, which has a heapless Vec of parsed options. This way, we can modify the options when needed.

The function of the Ipv6ExtHeader struct is now purely for parsing the ext header type and the length. It also returns a pointer to the data it holds, which must be parsed by the correct extension header representations.

codecov[bot] commented 11 months ago

Codecov Report

Merging #827 (53bdf43) into main (fa7fd3c) will increase coverage by 0.14%. Report is 5 commits behind head on main. The diff coverage is 97.22%.

@@            Coverage Diff             @@
##             main     #827      +/-   ##
==========================================
+ Coverage   79.65%   79.80%   +0.14%     
==========================================
  Files          72       72              
  Lines       27862    27991     +129     
==========================================
+ Hits        22193    22337     +144     
+ Misses       5669     5654      -15
Files Changed Coverage Δ
src/wire/mod.rs 71.96% <ø> (ø)
src/wire/ipv6hbh.rs 97.02% <97.02%> (ø)
src/iface/interface/ipv6.rs 91.38% <100.00%> (ø)

... and 3 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

whitequark commented 11 months ago

Hm, why are we using both heapless and managed?

whitequark commented 11 months ago

It looks like heapless was added in https://github.com/smoltcp-rs/smoltcp/commit/da1a2b2df0eafebb7fb92c00e56e88d533daa446, which is fair enough (I didn't investigate why the DNS server needs it; I assume it does) but in the core of the stack I'd rather only see managed.

thvdveld commented 11 months ago

I think that'll be hard to pass the managed buffer to the parser no? The parse function should get a managed buffer from the interface, so then the interface inner should have a managed buffer, so then we need to implement a function that gives a managed buffer to the interface, which we then have to check if it was given by the user or not, and so on. I think it's simpler using the heapless::Vec in the wire module. I agree that using managed buffers for fragmentation and socket buffers is a must.

whitequark commented 11 months ago

Yeah, on second thought you're right, go ahead.