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.

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

Add ControlMessage::Ipv6MulticastHopLimit #2074

Merged

Conversation

sgasse
Copy link
Contributor

@sgasse sgasse commented Jul 14, 2023

When sending IPv6 multicast packets with sendmsg, Linux does not use the hop limit set on the socket. Instead, the hop limit has to be specified for each individual message with ancillary data in a cmsg.

This commit adds the enum variant
ControlMessage::Ipv6MulticastHopLimit to specify the limit.

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.

The PR looks good but for a few things:

  • Why limit it to Linux? libc defines IPV6_HOPLIMIT for several operating systems.
  • It needs a CHANGELOG entry.
  • Ideally there would be a test, though that can be hard for some cmsg types.

@asomers
Copy link
Member

asomers commented Jul 15, 2023

Oh, and most of the CI failures should be fixed if you rebase on master.

@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch from a8df4d3 to 9adc8e2 Compare August 2, 2023 16:08
@sgasse
Copy link
Contributor Author

sgasse commented Aug 2, 2023

Thanks for the feedback @asomers !

  • Seeing with which hop limit a packet actually leaves the network interface is hard to test - if you have a suggestion for a test or an existing test to orient myself along, I am happy to write one.
  • Because I can only manually test this so far, I only added linux because I could not verify that it works on other systems myself. If I should add another group of devices (android or something) please let me know.
  • I added a changelog entry.

Unfortunately, there are still a bunch of CI failures which I think are not related to my patch despite rebasing on latest upstream/master 🤔

@asomers
Copy link
Member

asomers commented Aug 6, 2023

No, I don't have a good suggestion for how to write a test. It isn't always possible for weird network options. But I did fix the CI failures on master, so that should work if you rebase. And I suggest you go ahead and define this on every platform where libc defines IPV6_HOPLIMIT. That looks like haiku, Linux, Android, FreeBSD, DragonflyBSD, macos, and ios.

@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch from 9adc8e2 to e35d4ff Compare August 10, 2023 15:37
@sgasse
Copy link
Contributor Author

sgasse commented Aug 10, 2023

OK thanks 🙂 I updated the patch to include all the target_oses you mentioned. Where would I find the info for which target_os we have IPV6_HOPLIMIT defined? I tried to check in the libc crate but did not find cfg flags with the set that you mention 🤔

Patch is rebased (and tested after the rebase locally), fingers crossed that it gets through CI this time.

@asomers
Copy link
Member

asomers commented Aug 11, 2023

Yes, the libc crate is the correct place to check. This is what I see, with an up to date checkout:

> rg -N IPV6_HOPLIMIT ../libc/src/
../libc/src/unix/haiku/mod.rs
pub const IPV6_HOPLIMIT: ::c_int = 33;

../libc/src/unix/linux_like/mod.rs
pub const IPV6_HOPLIMIT: ::c_int = 52;

../libc/src/unix/bsd/apple/mod.rs
pub const IPV6_HOPLIMIT: ::c_int = 47;

../libc/src/unix/bsd/freebsdlike/mod.rs
pub const IPV6_HOPLIMIT: ::c_int = 47;

../libc/src/unix/nto/mod.rs
pub const IPV6_HOPLIMIT: ::c_int = 47;

../libc/src/unix/aix/mod.rs
pub const IPV6_HOPLIMIT: ::c_int = 40;

@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch 2 times, most recently from fca4475 to 8d7c5a4 Compare August 23, 2023 12:32
@sgasse
Copy link
Contributor Author

sgasse commented Aug 23, 2023

Ah using grep to find the mentions make sense, thanks 🙏

I guess I was too slow in resolving the merge conflict - there seem to be new CI issues with a transitive dependency on some runners:

error: failed to compile `cross v0.2.1`, intermediate artifacts can be found at `/tmp/cargo-installrZI4R6`

Caused by:
  package `addr2line v0.21.0` cannot be built because it requires rustc 1.65 or newer, while the currently active rustc version is 1.63.0

Is the right course of action to bump the MSRV here, or pinning some transitive dependency?

@asomers
Copy link
Member

asomers commented Aug 26, 2023

Yeah, I raised MSRV. And it's fixed now, so you can rebase.

@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch from 8d7c5a4 to 9437b27 Compare August 27, 2023 11:00
@sgasse
Copy link
Contributor Author

sgasse commented Aug 27, 2023

Thanks, should be ready now after the rebase 🙂

@asomers asomers added this pull request to the merge queue Aug 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 27, 2023
@asomers
Copy link
Member

asomers commented Aug 27, 2023

@sgasse there was a merge conflict.

@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch 2 times, most recently from fa314d2 to 45d0545 Compare August 28, 2023 07:36
@sgasse
Copy link
Contributor Author

sgasse commented Aug 28, 2023

Hm OK during a local rebase, I was not required to resolve anything manually. However seeing that you released version 0.27.0, I moved my addition to the changelog into a new Unreleased block.

@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch 2 times, most recently from 8c54f08 to 8aad97b Compare August 29, 2023 15:23
@sgasse
Copy link
Contributor Author

sgasse commented Aug 29, 2023

@asomers maybe now everything is ready for a merge? 🙂

@sgasse
Copy link
Contributor Author

sgasse commented Sep 27, 2023

@asomers would you be fine with merging this? It would be really useful for me; right now, I have to include this version with a git tag to be able to use the functionality. I think I resolved all issues that you raised, the changelog is up to date and CI is also happy.

@asomers
Copy link
Member

asomers commented Oct 1, 2023

@sgasse the changelog is out of date again, unfortunately. We really need to switch to recording it in a less merge-conflict-prone way.

When sending IPv6 packets with `sendmsg`, Linux does not use the hop limit
set on the socket. Instead, the hop limit has to be specified for each
individual message with ancillary data in a cmsg.

This commit adds the enum variant
`ControlMessage::Ipv6HopLimit` to specify the limit. The variant is
available on the `net` feature flag for Linux, MacOs, FreeBSD,
DragonflyBSD, Android, iOS and Haiku.
@sgasse sgasse force-pushed the sgasse/feature/cmsg_ipv6_multicast_hop branch from 8aad97b to c2297a0 Compare October 3, 2023 06:53
@sgasse
Copy link
Contributor Author

sgasse commented Oct 3, 2023

Ah OK 🙃 I have not tried it out, but maybe there are less merge conflicts if we always had the complete structure

## [Unreleased] - ReleaseDate

### Fixed

### Changed

### Added

above the release changes? I would hope that git can then figure out where to place changes even if other releases are done before. But not sure, how do other crates to that? 🤔

Nevertheless, updated the patch.

@SteveLauC SteveLauC added this pull request to the merge queue Oct 3, 2023
Merged via the queue into nix-rust:master with commit 14a2969 Oct 3, 2023
35 checks passed
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.

3 participants