Add UDP multicast simulation #223
Conversation
manifest
force-pushed
the
udp-multicast
branch
from
770edf2 to
7ae915a
Compare
manifest
force-pushed
the
udp-multicast
branch
5 times, most recently
from
b71e108 to
c4e3d0d
Compare
|
I’ve fixed clippy warnings and simplified tests for leaving multicast group. |
manifest
force-pushed
the
udp-multicast
branch
2 times, most recently
from
c0dcf7d to
1d2ee20
Compare
There was a problem hiding this comment.
Core approach seems reasonable to me.
Mostly my comments were nits around clarity of error messages and/or doc comments. Take them or leave them, none are blocking.
The only substantive thing I have questions about is, why aren't we verifying the bind addresses for ipv6 (only for ipv4 currently)?
| Ok(()) | ||
| } | ||
|
|
||
| fn verify_ipv6_bind_interface(interface: u32) -> Result<()> { |
There was a problem hiding this comment.
Is there a reason this can't just be combined into a verify_bind_interface check? Given that we are already using it to verify ipv6 for the bind() method? Anyway we still want to validate the protocol version matches? And we still want to validate that the interface is unspecified or loopback since Turmoil isn't doing anything with them so accepting anything else would be misleading callers?
There was a problem hiding this comment.
I believe there are few reasons why we can’t combine those functions. I've changed argument names of the verify_ipv4_bind_interface() to make that clear.
The first argument of the verify_ipv4_bind_interface() function is an interface address. We verify that it is unspecified or loopback, and test the fact that its protocol version matches the protocol version of the second argument, which is destination address (or host address, different names for the same address). We do not test the second argument (destination address).
The first argument of verify_ipv6_bind_interface() function has type u32. It’s not an IP address. We can’t test it for being unspecified or loopback, or for protocol version.
There was a problem hiding this comment.
IE, I'm having trouble understanding enforcing 0 rather than the Ipv6Addr::UNSPECIFIED or Ipv6Addr::LOCALHOST
There was a problem hiding this comment.
Ah, comment race, sorry... digesting your response, thanks
There was a problem hiding this comment.
Thanks for your clarification. I see, the APIs are just different across ipv4 and ipv6 for UDP socket - https://doc.rust-lang.org/std/net/struct.UdpSocket.html#method.join_multicast_v6
The address must be a valid multicast address, and interface is the index of the interface to join/leave (or 0 to indicate any interface).
A bit awkward, but totally outside of the scope of this CR. Your decision makes sense in that context, thanks for your patience.
There was a problem hiding this comment.
I've changed argument names of the verify_ipv4_bind_interface() to make that clear.
Thanks for this change, added clarity
|
Thanks for the quick turnaround on review feedback! Will move conversation back to the issue: #208 |
No description provided.