smoltcp icon indicating copy to clipboard operation
smoltcp copied to clipboard

(Incomplete) Implement `join_multicast_group()` for IPv6

Open jgallagher opened this issue 3 years ago • 2 comments
trafficstars

This is a preliminary and incomplete start to adding support for IPv6 multicast groups. This PR only adds joining groups via sending an initial MLDv2 report packet; it doesn't support resending the change report, leaving groups, responding to queries, etc. I wanted to open this PR first both to keep the review burden lighter and to get any feedback on the implementation that could affect implementing the remaining features.

On that last point, there are a few things in this PR that I'm not thrilled with and would be happy for any suggestions. I think all of them derive from difficulty crafting a fully-formed IpPacket to pass to dispatch_ip(). For the MLDv2 report, the specific difficulties and paths I took to deal with them were:

  1. We are required to set the router alert option in an ipv6 hop-by-hop header, which requires us to be able to craft a packet with a hop-by-hop header in the first place! I added a new case to IpPacket (IpPacket::HopByHopIcmpv6), but that doesn't feel quite right.
  2. Ipv6HopByHopRepr's options field requires a borrowed slice. I couldn't see an easy way to get from a pair of Ipv6OptionReprs to a borrowed slice, so I punted on this and defined Ipv6HopByHopRepr::MLDV2_ROUTER_ALERT as a constant.
  3. Similarly, MldRepr::Report requires a borrowed slice for its data (the address records); I couldn't see an easy way to get from an MldAddressRecordRepr to that borrowed data, so I added an MldRepr::ReportRecordReprs case which holds a slice of MldAddressRecordRepr. This definitely seems wrong. It also might be still have similar problems as the borrowed byte slice when we need to craft packets with multiple address records in them.

In addition to the unit tests present, I've tested this locally on a microcontroller running smoltcp, and I've confirmed I can communicate with it via a multicast address through the switch it's connected to, so I'm reasonably confident that what's here is correct (albeit incomplete and a little hacky, as noted above).

jgallagher avatar Apr 19 '22 15:04 jgallagher

re the difficulties: Yes, IpPacket is not "scaling" well, this happens with every new feature added. Your solutions look reasonable. (I've been thinking for a while about refactoring it so that all the packet emitters (sockets, and the IGMP code) gets access to the buffer and write their packets there directly, but that has other downsides...)

From a quick look, the PR looks OK. Could you rebase+squash?

Dirbaio avatar May 19 '22 03:05 Dirbaio

friendly ping @jgallagher :)

Dirbaio avatar Jul 29 '22 10:07 Dirbaio