-
Notifications
You must be signed in to change notification settings - Fork 279
Added Socket::set_multicast_if_v4_n to set interface by index (#458) #582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
940165e to
f42637a
Compare
f42637a to
77eb5c7
Compare
|
Added feature = "all" as setting multicast interface by index is not supported on Windows and MacOSX. |
77eb5c7 to
159c832
Compare
159c832 to
3fac89f
Compare
3fac89f to
817cfb1
Compare
|
Rebased to current master, waiting for review/approval. |
817cfb1 to
6e7e593
Compare
|
Rebased to current master (again), waiting for review/approval. |
|
Any thoughts on expanding this to set both an address and an if index? It seems like we're pinned to using either or in this PR, but we could potentially want both (e.g. sending multicast traffic from a particular address on the interface). |
Thomasdezeeuw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment, but I'm not sure about this pr. I don't use these APIs and currently don't have the time to look into them properly. For this to be merged it would be nice if someone with experience with the options could confirm they work as expected. This also needs a test.
src/socket.rs
Outdated
| "Interface index out of bounds", | ||
| )); | ||
| } | ||
| Ipv4Addr::from_bits(*index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. The enum variant clearly say Index, but we're using an IPv4 address. I think we shouldn't support the index here and recommend to use the Address instead for NetBSD, but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is correct (although I have to admit it is probably confusing). The NetBSD API seems to be a bit "unusual" for setting the multicast interface by index. Let me cite from the NetBSD man page (https://man.netbsd.org/NetBSD-7.0/ip.4):
The IP_MULTICAST_IF option overrides the default for subsequent transmissions from a given socket:
struct in_addr addr;
setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, &addr, sizeof(addr));
where "addr" is the local IP address of the desired interface [...]. An application may also specify an alternative to the default network interface by index:
struct uint32_t idx = htonl(ifindex);
setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, &idx, sizeof(idx));
where "ifindex" is an interface index as returned by if_nametoindex(3).
So, while Linux, FreeBSD, Android and Fuchsia use a special struct mreqn (which has a separate field for the interface index) if they want to set the multicast interface by index, NetBSD uses a struct in_addr to set the address by IP and an uint32_t (in network byte order.) to set the address by interface index. struct in_addr and uint32_t both are 4 bytes in size, so the kernel cannot distinguish the two cases from the size parameter. Looking into the kernel code shows that IP addresses in subnet 0.0.0.0/8 are interpreted as interface index (see https://github.com/NetBSD/src/blob/trunk/sys/netinet/ip_output.c#L1691). This is the reason why I converted the interface to an address and used this one with the setsockopt call.
But I agree, the implementation is probably easier to understand if the code aligns to the examples given in the NetBSD manual page. I provided a modified version which does so.
6e7e593 to
086130a
Compare
086130a to
2638437
Compare
Implementation for Socket::set_multicast_if_v4_n (similar to Socket::join_multicast_v4_n) which allows to set interface by index or address (via struct InterfaceIndexOrAddress), for Linux/Android/Fuchsia/FreeBSD/NetBSD. (Other OSes seem to have no support for this.)