IPv4 fragmentation (for outgoing)

[thvdveld]

Almost ready for review, just need to clean up some things :tada:

[thvdveld]

I think it is ready for review now.

[thvdveld]

I did an oopsie.

[Dirbaio]

• I don't think Ipv4Repr / IpRepr should have the fragmentation fields. It's handled at the lowest level in Interface, so all the higher layers (socket, etc) only deal with unfragmented packets. Having the fields in Reprs forces all the code to deal with them, which makes tests more verbose and worsens code size a bit (I've measured +1kb approx in a firmware without fragmentation enabled)
• Could you do the buffer_len -> header_len rename in a separate PR? It makes the diff too big.

[thvdveld]

I removed the panicking behaviour, renaming is removed (will do that in another PR), renamed the buffers (better names) and removed the fragmentation fields in the Repr.

[KOLANICH]

@thvdveld, I'm an author of an alternative implmof IPv4 fragmentation, #672. I'm a newbie in networks and emhedded, so I had done it differently.

0. I don't use a separate cache for output fragments, instead I just split an IPv4 packet when it is sent. I basically just serialize it into bytes, parse again (I guess it should be zero-cost since the things we are doing are just like interpreting a pointer to memory as a pointer to a struct), read its size, rip IP header using the size, split the rest into chunks, send each chunk as a payload of a raw IPv4 packet.
1. Could you clarify why do we need a reassemble buffer for output packets?
2. I have split raw IPv4 packets into a separate feature from raw sockets, since I use raw IPv4 packets and don't use raw sockets.
3. In my impl transmitting fragmented IPv4 packets is a distinct feature from receiving fragmented IPv4 packets, since we don't need to reassemble the packets we send (why do we?).
4. In my impl fragmentation flags are exposed in the Repr as a enum taking 3 values. Values DontFragment and LastFragment are used by upper level protocols to allow or disallow fragmentation, for example usually fragmentatiin for TCP is undesireable.
5. My impl lacks tests, I guess I may try to transplant the tests from yours.

[thvdveld]

Thanks for your PR! Some answers:

0 and 1. The reason why I have a separate buffer for the outgoing fragments is because I didn't do the fragmentation in the socket_egress function. I did it in the dispatch_ipv4 function and since I don't have access to the device in that function, I cannot call device.transmit(). Maybe I should move the fragmentation in my implementation into the egress function (what do you think @Dirbaio?). The other reason why I needed the output buffer is because of the platform that I'm using (which is a Zolertia RE-Mote). My device trait implementation I have is very simple and only has a buffer size equal the MTU of IEEE802.15.4. This means that device.transmit() will return Error::Exhausted the second time transmit is called in the loop, meaning that packets other than the first fragment will be lost. Also, since smoltcp is also used on devices that don't have allocators, using Vec is not possible. This was not checked in CI for your PR because your feature flag was not added to the CI compile tests that don't use std. 2. Not sure what you mean with that. socket-raw should not care about the IP layer (I think), since sockets can be used with IPv6 and IPv4. 3. That's a good point, maybe I should do that as well (but not sure if it is really necessary). 4. Adding those flags to the Repr has some implications. Firstly, this means that we need to add them everywhere, even on places where we really don't care about it. Secondly, it adds a lot of memory overhead which is not really necessary. @Dirbaio had a comment on my implementation where he measured the memory overhead, and it was something around 1kb. It's something that is not really needed in the Repr. For TCP we shouldn't really care because TCP has his segmentation mechanism. 5. I also didn't implemented tests (I probably should). I just modified the examples such that I was able to test it that way.

[KOLANICH]

0. and 1. Thanks for the explaination.
1. IPv4 raw packets were under socket-raw feature.

2. Secondly, it adds a lot of memory overhead which is not really necessary.

Yes, that was the primary reason because of which I had put it under a separate feature. When the feature is not enabled, the field gets missing from the Repr.

5. I also didn't implemented tests (I probably should). I just modified the examples such that I was able to test it that way.

I tested using my (for now pretty unfinished) python bindings to smoltcp. Basically just sent an UDP packed and set MTU to 42 bytes.

[thvdveld]

I can't seem to find what you mean with the socket-raw feature:

https://github.com/smoltcp-rs/smoltcp/blob/6b8b667be5878fbf288365df9260996116b81fa6/Cargo.toml#L35-L73

[KOLANICH]

rg socket-raw

Cargo.toml
56:"socket-raw" = ["socket"]
71:  "socket-raw", "socket-icmp", "socket-udp", "socket-tcp", "socket-dhcpv4", "socket-dns",
113:required-features = ["std", "medium-ethernet", "medium-ip", "phy-tuntap_interface", "proto-ipv4", "socket-raw", "socket-udp"]
117:required-features = ["std", "medium-ethernet", "medium-ip", "phy-tuntap_interface", "proto-ipv4", "proto-dhcpv4", "socket-raw"]

README.md
198:### Features `socket-raw`, `socket-udp`, `socket-tcp`, `socket-icmp`, `socket-dhcpv4`

src/iface/interface.rs
574:    #[cfg(feature = "socket-raw")]
593:            #[cfg(feature = "socket-raw")]
626:            #[cfg(feature = "socket-raw")]
1109:                #[cfg(feature = "socket-raw")]
1895:    #[cfg(feature = "socket-raw")]
1933:        #[cfg(feature = "socket-raw")]
1935:        #[cfg(not(feature = "socket-raw"))]
1973:            #[cfg(feature = "socket-raw")]
2063:        #[cfg(feature = "socket-raw")]
2065:        #[cfg(not(feature = "socket-raw"))]
4523:    #[cfg(all(feature = "proto-ipv4", feature = "socket-raw"))]
4598:    #[cfg(all(feature = "proto-ipv4", feature = "socket-raw", feature = "socket-udp"))]

src/socket/mod.rs
23:#[cfg(feature = "socket-raw")]
60:    #[cfg(feature = "socket-raw")]
77:            #[cfg(feature = "socket-raw")]
126:#[cfg(feature = "socket-raw")]

src/lib.rs
79:#![cfg(not(any(feature = "socket-raw",
103:        feature = "socket-raw",
111:compile_error!("If you enable the socket feature, you must enable at least one of the following features: socket-raw, socket-udp, socket-tcp, socket-icmp, socket-dhcp, socket-dns");

[thvdveld]

The only time when socket-raw and proto-ipv4 are used together is in a specific test. Like I said, raw sockets should not care about the IP layer with regard to feature flags.

[thvdveld]

Friendly ping @Dirbaio :)

[bors[bot]]

Build failed:

• tests

[Dirbaio]

small conflict with #679, FTFY

bors r+

[bors[bot]]

Build succeeded:

• clippy
• fmt
• fuzz
• tests