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 Ipv{4,6}PacketInfo support to ControlMessage for send{m,}msg. #1222

Merged
merged 1 commit into from Jun 27, 2020
Merged

Add Ipv{4,6}PacketInfo support to ControlMessage for send{m,}msg. #1222

merged 1 commit into from Jun 27, 2020

Conversation

isomer
Copy link
Contributor

@isomer isomer commented Apr 26, 2020

This adds Ipv4PacketInfo and Ipv6PacketInfo to ControlMessage, allowing these to be used with sendmsg/sendmmsg.

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.

Can you please add tests for the new functionality?

CHANGELOG.md Show resolved Hide resolved
src/sys/socket/mod.rs Outdated Show resolved Hide resolved
src/sys/socket/mod.rs Show resolved Hide resolved
@isomer
Copy link
Contributor Author

isomer commented Apr 27, 2020

Can you please add tests for the new functionality?

Ah, it took me a while to figure out where the tests were. Duh. Now I've found them, I've added some simple tests.

These features are most useful when there are multiple IP addresses on the machine (to force which IP address is used), but I don't think I can assume that the test hosts will have any IPs other than localhost. So I've forced localhost (despite the kernel probably choosing localhost anyway).

At least under Linux, the kernel is very picky about these options, so if something is wrong, it will generally fail with EINVAL, so I think this should be fine.

So, tl;dr: Done.

@isomer
Copy link
Contributor Author

isomer commented Apr 27, 2020

I'm still learning rust. I don't understand why the freebsd test is failing – it should be [cfg]'d to not even compile that section of the code on freebsd.

It's late here, so I'm going to bed, I'll pick this up again and try and figure out what's going on tomorrow evening.

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.

It's lame that Travis doesn't have any IPv6 support. According to their docs, they only have that for arm64, ppc64le, and s390x. So you'll have to conditionally disable that test.

// This creates a (udp) socket bound to localhost, then sends a message to
// itself but uses Ipv4PacketInfo to force the source address to be localhost.
//
// This would be a more interesting test if we could assume that the test host
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the CI systems as well as most dev systems won't have multiple IP addresses. But if you really want to make a great demonstration of the feature, you could add a binary in the examples/ directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think a good demo here is particularly useful. These are low level APIs that people shouldn't be using directly. Somewhere there should be a crate that abstracts over all of this and provides something like set_local_address() and figures out what the correct underlying implementation to use (of which these are only one implementation). For example, for FreeBSD, Ipv6PacketInfo is the correct answer, but there's a different solution for IPv4.

I just thought I'd document why the test seems to be particularly inane. The test should be useful, if anything goes wrong, the kernel will reject it and the test will catch that.

@isomer
Copy link
Contributor Author

isomer commented Apr 29, 2020

It's lame that Travis doesn't have any IPv6 support. According to their docs, they only have that for arm64, ppc64le, and s390x. So you'll have to conditionally disable that test.

I'm surprised they don't even have support for the loopback interface. I've changed the code to detect the error when there is no IPv6 loopback and skip the test. I hope over time the environments will eventually gain IPv6 support, so this will start be tested there when that happens without a human having to keep track of what is going on.

@isomer
Copy link
Contributor Author

isomer commented Apr 29, 2020

There's one failing test, which I think is an unrelated spurious flake - it appears to be to do with aio on one platform. I can't figure out how to get travis to retry, and unfortunately I've run out of day again.

@asomers
Copy link
Member

asomers commented May 2, 2020

I believe that aio failure is caused by a bug in glibc. I've restarted it.

@asomers
Copy link
Member

asomers commented May 2, 2020

I think this PR looks good. If the Travis test passes, could you please squash your commits?

@asomers
Copy link
Member

asomers commented May 7, 2020

Sorry, but merging master into your branch screwed up the PR. Now other peoples' commits appear to be part of this PR. You'll have to rebase, not merge.

This adds Ipv4PacketInfo and Ipv6PacketInfo to ControlMessage,
allowing these to be used with sendmsg/sendmmsg.

This change contains the following squashed commits:

Add Ipv{4,6}PacketInfo to ControlMessage.

Add documentation links to Ipv{4,6}PacketInfo

Add changelog entry for Ipv{4,6}PacketInfo

Add link to PR in the Changelog.

Add extra build environments.

Add tests for Ipv{4,6}PacketInfo.

Swap #[test] and #[cfg]

The CI appears to be running the test, even though it's not cfg'd for
that platform.  I _think_ this might be due to these being in the wrong
order.  So lets try swapping them.

s/freebsd/netbsd/ for Ipv4PacketInfo

netbsd supports in_pktinfo, not freebsd.

Fix the cfg for Ipv{4,6}PacketInfo usage.

Ah, I see what I did wrong.  I had fixed the definitions, but I had the
wrong cfg() in the usage.  This has the usage match the definitions.

Change SOL_IPV6 to IPPROTO_IPV6.

FreeBSD doesn't have SOL_IPV6, but does have IPPROTO_IPV6, and the two
constants are defined as being equal.  So change to use IPPROTO_IPV6.

Skip Ipv6PacketInfo test if v6 is not available.

If IPv6 is not available, then when we try and bind to ip6-localhost,
we'll get a EADDRNOTAVAIL, so skip the test.

This should mean that the test will run on any machine that has a v6
loopback address.

More architecture cfg() fixes.

These all need to be the same, and they were not.  Make them them all
the same.  Attempt III.

Fix up mismatched cfg's again.

Take IV.  Make sure the cfg's that use a enum variant match the enum
definition.
@isomer
Copy link
Contributor Author

isomer commented Jun 26, 2020

With some help from tazjan I've finally managed to get this properly cleaned up. Please Take Another Look?

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.

It all looks good now!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 27, 2020

Build succeeded:

@bors bors bot merged commit 2c42b30 into nix-rust:master Jun 27, 2020
bltavares added a commit to bltavares/nix that referenced this pull request Jun 30, 2020
The commit nix-rust#1222 added the very
useful Ipv4PktInfo to allow `sendmsg` to define the origin of the ip.

Unfortunattely, it didn't add the struct to Android target devices as
well. This commit adds the `target_os = "android"` checks on the same
place to allow the compilation to work for the following archs tested:

- `cross build --target aarch64-linux-android`
- `cross build --target x86_64-linux-android`
- `cross build --target armv7-linux-androideabi`
bltavares added a commit to bltavares/nix that referenced this pull request Jul 4, 2020
The commit nix-rust#1222 added the very
useful Ipv4PktInfo to allow `sendmsg` to define the origin of the ip.

Unfortunattely, it didn't add the struct to Android target devices as
well. This commit adds the `target_os = "android"` checks on the same
place to allow the compilation to work for the following archs tested:

- `cross build --target aarch64-linux-android`
- `cross build --target x86_64-linux-android`
- `cross build --target armv7-linux-androideabi`

Also introduces iOS to allow using on libs for those platforms
bors bot added a commit that referenced this pull request Jul 7, 2020
1265: Expose IP_PKTINFO Control Message on Android and iOS r=asomers a=bltavares

The commit #1222 added the very
useful Ipv4PktInfo to allow `sendmsg` to define the origin of the ip.

Unfortunattely, it didn't add the struct to Android target devices as
well. This commit adds the `target_os = "android"` and `target_os =" ios"` checks on the same
place to allow the compilation to work for the following archs tested:

- `cross build --target aarch64-linux-android`
- `cross build --target x86_64-linux-android`
- `cross build --target armv7-linux-androideabi`
- `cross build --target aarch64-apple-ios`
- `cross build --target x86_64-apple-ios`

Co-authored-by: Bruno Tavares <connect+github@bltavares.com>
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.

None yet

2 participants