Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix setting sockopt::IpMulticastTtl #2072

Merged
merged 2 commits into from
Dec 9, 2023
Merged

Fix setting sockopt::IpMulticastTtl #2072

merged 2 commits into from
Dec 9, 2023

Conversation

sgasse
Copy link
Contributor

@sgasse sgasse commented Jul 13, 2023

The socket option IpMulticastTtl was implemented with u8 as type however i32 seems to be the correct type.

@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from 24fd8da to a57e556 Compare July 13, 2023 12:23
@sgasse sgasse mentioned this pull request Jul 13, 2023
Closed
@asomers
Copy link
Member

asomers commented Jul 15, 2023

The socket option IpMulticastTtl was implemented with u8 as type however i32 seems to be the correct type.

Why do you say that? It's documented as being u8 on FreeBSD, at least.

@asomers
Copy link
Member

asomers commented Jul 17, 2023

Oh, I just saw the more detailed description in the issue. Please link the issue in your commit message. And yes, this does seem to be a difference between platforms. This sockopt was added way back in 2015 with the first batch of sockopt macros, in 979faf0 . There wasn't good CI at the time, and no tests for that sockopt, so nobody noticed that it was broken.
It would be nice if you could add a test for this one.

@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from a57e556 to 91ce275 Compare July 18, 2023 15:42
@sgasse
Copy link
Contributor Author

sgasse commented Jul 18, 2023

Thanks for the context! I pushed and opened the PR first when there was no issue but now I included the issue in the commit message. My initial commit also included a new test test_multicast_ttl_opts which fails on linux when using u8. It tries to set sockopt::IpMulticastTtl on both a v4 and v6 socket. Is that OK as a test or do we need something more?

I rebased on the latest master and added config guards, using u8 for all BSD-like systems and i32 everywhere else. Could you trigger another CI run? Then we can hopefully tell which other systems need u8 instead of i32.

@asomers
Copy link
Member

asomers commented Jul 18, 2023

@sgasse CI should be automatic. Maybe Cirrus had a hiccup? Try force-push again.

@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch 3 times, most recently from 86e4e77 to e1b44db Compare July 19, 2023 09:06
@sgasse
Copy link
Contributor Author

sgasse commented Jul 19, 2023

Alright I guess it was a hiccup in Github itself - the force-push yesterday did not make it through to the PR for 5mins ^^

After the rebase, some more CI jobs pass but some still have this error on mutable reference not used mutable 🤔 And the test I added fails on FreeBSD both with u8 and with libc::c_int which is really strange.

@asomers
Copy link
Member

asomers commented Aug 6, 2023

There are three reasons why that test is failing for you. The first is that it really should be a u8, not a u32. Your PR changes that. But the second is a bug in Nix dating to 979faf0 that has been hitherto unnoticed because of insufficient test coverage. Even though the sockopt is defined as using u8, Nix was passing 4 as its length to libc::setsockopt. I've pushed a change to fix that. The third reason is that IP_MULTICAST_TTL doesn't work on IPv6 sockets, at least not on FreeBSD. You should remove that part from your test. Also, you may need to rebase to fix CI.

@sgasse
Copy link
Contributor Author

sgasse commented Aug 10, 2023

OK thanks for the thorough explanation 🙏 I will take a look tomorrow and update.

@asomers asomers added this to the 0.26.3 milestone Aug 11, 2023
@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from f880bfa to ae410a4 Compare August 30, 2023 11:15
@sgasse sgasse changed the title Fix wrong type for sockopt::IpMulticastTtl Fix setting sockopt::IpMulticastTtl Aug 30, 2023
@sgasse
Copy link
Contributor Author

sgasse commented Aug 30, 2023

I took the time now to check it and looks good, your fix solves the problem entirely @asomers and it works with u8 also on Linux and Android. Therefore, I renamed this PR, reduced my commit to just adding tests, amended your commit with a changelog entry and moved it before mine in the chain. I kept the IPv6 socket tests on Linux and Android because they work there.

There seems to be some congestion at CirrusCI (the failing test lost connection to the runner), but I will force-push again later and hope that the tests pass then.

@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from ae410a4 to 507f22e Compare August 30, 2023 13:56
@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from 507f22e to eb56ff3 Compare October 5, 2023 18:11
@sgasse
Copy link
Contributor Author

sgasse commented Oct 5, 2023

@asomers I think this would also be ready for merge.

asomers
asomers requested changes Oct 5, 2023
View reviewed changes
test/sys/test_sockopt.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from eb56ff3 to 76c99cc Compare October 11, 2023 15:01
@asomers
Copy link
Member

asomers commented Nov 26, 2023

Could you please try rebasing this PR to fix CI and resolve the conflict?

@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from 76c99cc to ab7f996 Compare November 27, 2023 17:28
@sgasse
Copy link
Contributor Author

sgasse commented Nov 27, 2023

Sure, updated 🙂

@sgasse
Copy link
Contributor Author

sgasse commented Nov 29, 2023

Should be ready again @asomers ;)

@asomers
Copy link
Member

asomers commented Dec 8, 2023

I'm afraid there's another conflict, @sgasse . Could you please rebase?

asomers and others added 2 commits December 9, 2023 11:24
@asomers @sgasse
Fix the value of optlen as passed to libc::setsockopt
0a0d0f2 
Ever since the sockopt macro was written in
979faf0 we've been passing the wrong
size for u8 and usize sockopts.
@sgasse
Add tests for sockopt::IpMulticastTtl nix-rust#2073
4f57ac3 
On Linux and Android, sockopt::IpMulticastTtl can be set for both IPv4
and IPv6 sockets, but not on FreeBSD.
@sgasse sgasse force-pushed the sgasse/fix/sockopt_ipmulticastttl branch from ab7f996 to 4f57ac3 Compare December 9, 2023 10:32
@sgasse
Copy link
Contributor Author

sgasse commented Dec 9, 2023

Sure, I guess that conflict came from my multicast hops PR. Rebased and pushed again 🙂

@asomers asomers added this pull request to the merge queue Dec 9, 2023
Merged via the queue into nix-rust:master with commit a09971f Dec 9, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.26.3
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sgasse @asomers