Skip to content

add IP_MULTICAST_IF for sockopt #1363

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

Closed
wants to merge 7 commits into from

Conversation

keepsimple1
Copy link

@keepsimple1 keepsimple1 commented Dec 15, 2020

IP_MULTICAST_IF is not supported by sockopt in nix. See issue #1360 .

According to Linux man page, IP_MULTICAST_IF is used in this way:

quote:

IP_MULTICAST_IF (since Linux 1.2)
              Set the local device for a multicast socket.  The argument for
              setsockopt(2) is an ip_mreqn or (since Linux 3.5) ip_mreq
              structure similar to IP_ADD_MEMBERSHIP, or an in_addr struc‐
              ture.  (The kernel determines which structure is being passed
              based on the size passed in optlen.)  For getsockopt(2), the
              argument is an in_addr structure.

Please review this patch. Also I have a question: how do we add automated testing for such sockopt in nix? I've added a test case in my own code that uses nix and it works, but don't know about the practice of testing inside nix. Thanks.

Update: 12/24/2020

The original approach of defining an Enum did not work well because #[repr(transparent)] does not support Enum. Moreover, my use cases only care about in_addr value, which is the most common AFAIK. So I refactored the patch to address the major cases I actually understand, and added a test case.

Also, based on the Github Actions results, I excluded the patch from the (OS, env, arch) that failed with error ENOPROTOOPT.

@asomers
Copy link
Member

asomers commented Dec 16, 2020

Nix doesn't currently use anything fancy like mocking for tests. Just try to exercise the socket option. Don't write an exhaustive test suite for it; that's a job for the kernel maintainers. Nix's test suite should do just enough to ensure that the bindings are correct.

@keepsimple1
Copy link
Author

Thanks @asomers for your answers. I've updated the diff and the PR Description, adding a test case. Please take a look when you have time.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My man page says The use of IP_MULTICAST_IF is not recommended, as multicast memberships are scoped to each individual interface. It is supported for legacy use only.... Given that, we probably should't add Nix support for it. Is there something you need it for that can't be done any other way?

@@ -233,6 +233,17 @@ cfg_if! {
sockopt_impl!(SetOnly, Ipv6DropMembership, libc::IPPROTO_IPV6, libc::IPV6_LEAVE_GROUP, super::Ipv6MembershipRequest);
}
}
cfg_if! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cfg_if is helpful when you expect to have else or else if blocks. But in cases like this, it's simpler to do:

#[cfg(...)]
sockopt_impl!(...)

/// `ip_mreqn` or `ip_mreq` structure are not supported at this point, one
/// reason is that if we use enum to represent all possible structures,
/// `#[repr(transparent)]` does not support enum.
#[repr(transparent)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the option work with IPv6? If so you should use an enum type. Or maybe define different Nix sockopts, one for each supported address type.

@keepsimple1
Copy link
Author

My man page says The use of IP_MULTICAST_IF is not recommended, as multicast memberships are scoped to each individual interface. It is supported for legacy use only.... Given that, we probably should't add Nix support for it. Is there something you need it for that can't be done any other way?

Could you please share where I can find this man page you referred to? Thanks.

@keepsimple1
Copy link
Author

https://www.freebsd.org/cgi/man.cgi?query=ip&apropos=0&sektion=4&manpath=FreeBSD+12.2-RELEASE+and+Ports&arch=default&format=html

Thanks. Looks like FreeBSD deems it legacy (while Linux seems not).

And I found a workaround in my project at this point. I will close this PR for now until someone found its use again.

@keepsimple1 keepsimple1 closed this Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants