Add cmsg PKTINFO for IPv4 and IPv6. #990

pusateri · 2018-12-04T06:02:00Z

Replaces #891 and attempts to address all previous concerns.

pusateri · 2018-12-04T06:07:23Z

FreeBSD has IP_RECVIF and IP_RECVDSTADDR/IP_SENDSRCADDR to accomplish the same thing as IP_PKTINFO. I haven't added that yet. I will once this is committed.

pusateri · 2018-12-04T06:20:53Z

OpenBSD has IPV6_PKTINFO but cargo test fails before these changes so I did not yet include OpenBSD cfg support.

pusateri · 2018-12-04T15:31:47Z

Not sure if test_recv_ipv6pktinfo() should pass if there is no IPv6 address on the loopback interface. ::1 is missing on the amd64_fbsd11 buildbot automated test. The other alternative is to mark the test as ignore and let is only be run intentionally.

asomers · 2018-12-04T21:14:02Z

CHANGELOG.md

@@ -95,6 +95,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#857](https://github.com/nix-rust/nix/pull/857))
 - Added `request_code_write_int!` on FreeBSD/DragonFlyBSD
  ([#833](https://github.com/nix-rust/nix/pull/833))
+- Added PKTINFO cmsg support on Android/iOS/Linux/MacOS.
+- Added V6PKTINFO cmsg support on Android/FreeBSD/iOS/Linux/MacOS.


You'll need to move the changelog entries into the "Unreleased" section, and also add a link to the PR.

asomers · 2018-12-04T21:22:27Z

src/sys/socket/mod.rs

+            ControlMessage::Ipv4PacketInfo(pktinfo) => {
+                mem::size_of_val(pktinfo)
+            },
+            #[cfg(any(target_os = "android", target_os = "freebsd", target_os = "ios", target_os = "linux", target_os = "macos"))]


Can you please wrap to 80 columns?

pusateri · 2018-12-04T23:21:17Z

mips-unknown-linux-gnu looks like an endian problem. Not sure how deep. Index probably should have been 1 and it was 1000000 (hex).

pusateri · 2018-12-04T23:25:10Z

I tested on stable-armv7-unknown-linux-gnueabihf, Debian GNU/Linux 8.11 (jessie) with stable 1.30.0 and it passed. Not sure what's different with the test apparatus other than it using 1.24.1.

asomers · 2018-12-05T00:07:45Z

I tested on stable-armv7-unknown-linux-gnueabihf, Debian GNU/Linux 8.11 (jessie) with stable 1.30.0 and it passed. Not sure what's different with the test apparatus other than it using 1.24.1.

Were you actually running Debian on an arm? The test apparatus uses qemu to emulate the embedded architectures, and qemu's got some bugs.

pusateri · 2018-12-05T00:23:32Z

I tested on stable-armv7-unknown-linux-gnueabihf, Debian GNU/Linux 8.11 (jessie) with stable 1.30.0 and it passed. Not sure what's different with the test apparatus other than it using 1.24.1.

Were you actually running Debian on an arm? The test apparatus uses qemu to emulate the embedded architectures, and qemu's got some bugs.

Yes. I compiled and tested on real armv7 hardware. It is a solid-run.com Hummingboard.

Linux hum 3.14.79-fslc-imx6-sr #1 SMP Fri Oct 20 18:16:19 UTC 2017 armv7l GNU/Linux

asomers · 2018-12-05T16:42:42Z

Looks like you're getting EPFNOSUPPORT on many of the QEMU platforms. You can either disable the test for those platforms, or else accept EPFNOSUPPORT (and print a warning) in the test.

pusateri · 2018-12-05T17:26:50Z

Looks like you're getting EPFNOSUPPORT on many of the QEMU platforms. You can either disable the test for those platforms, or else accept EPFNOSUPPORT (and print a warning) in the test.

Yeah, I probably shouldn't be using unwrap() as much. It's probably when scanning the interfaces for addresses that it's failing. It's too bad that I can't get more info from it. I will try setting up QEMU and seeing if I can narrow it down instead of randomly removing unwrap() until I find it. :)

pusateri · 2018-12-06T02:37:51Z

Cool. All that's left to fix is the big endian problem.

pusateri · 2018-12-11T22:40:46Z

After some trouble, I got Ubuntu 16.04 LTS installed on a PowerMac Dual G5 and ran the pktinfo tests natively and both IPv4 and IPv6 tests passed. Maybe the endian bug is in the qemu emulator? I think the code is ok and don't have any more changes to make.

pusateri · 2018-12-11T22:42:08Z

This leaves us in a bad situation for testing though. I tested with version 1.31.0. I will test with 1.24.1 and verify since that's what the emulator is running.

asomers · 2018-12-11T22:44:20Z

There definitely are bugs in QEMU. That's why many tests are disabled for the QEMU platforms. It's not necessary for this PR that you fix QEMU; just make sure the tests pass on real platforms and all checkmarks are green.

pusateri · 2018-12-11T22:55:18Z

The powerpc tests passed on 1.24.1 as well. Currently, other tests are disabled for qemu bugs:

// qemu's handling of multiple cmsgs is bugged, ignore tests on non-x86
// see https://bugs.launchpad.net/qemu/+bug/1781280
#[cfg_attr(not(any(target_arch = "x86_64", target_arch = "x86")), ignore)]

Should I turn my tests off for powerpc and mips?

asomers · 2018-12-11T23:01:27Z

That's what I would do.

asomers · 2018-12-12T15:04:21Z

I'm afraid that you'll need to rebase. You somehow merged several other PRs into this one.

asomers · 2018-12-13T16:21:27Z

CHANGELOG.md

@@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]
+- Added PKTINFO cmsg support on Android/iOS/Linux/MacOS.
+- Added V6PKTINFO cmsg support on Android/FreeBSD/iOS/Linux/MacOS.
+- ([#990](https://github.com/nix-rust/nix/pull/990))


The PR doesn't get its own bullet. These three lines should either all share a bullet, or the "Added" lines should each get their own and each have the PR link.

asomers · 2018-12-13T16:41:52Z

test/sys/test_socket.rs

+        .expect("Mutex got poisoned by another test");
+
+    let lo_ifaddr = loopback_v4addr();
+    let (lo_name, lo) = match lo_ifaddr {


Could you simplify the test by hardcoding this to ("lo0", "127.0.0.1")?

asomers · 2018-12-13T16:53:17Z

test/test.rs

@@ -77,4 +77,6 @@ lazy_static! {
    pub static ref PTSNAME_MTX: Mutex<()> = Mutex::new(());
    /// Any test that alters signal handling must grab this mutex.
    pub static ref SIGNAL_MTX: Mutex<()> = Mutex::new(());
+    /// Any test sending packets over the loopback interface
+    pub static ref LOOP_MTX: Mutex<()> = Mutex::new(());


Is this really necessary? Why not just bind each test to a different port?

asomers · 2018-12-13T16:54:03Z

test/sys/test_socket.rs

+        );
+    });
+
+    let slice = [1u8, 2u8, 3u8, 4u8, 5u8, 6u8, 7u8, 8u8];


Pro tip: You can leave out the "u8" on the second and subsequent elements.

Do you want me to fix the versions above in the other existing tests or just this new one?

pusateri · 2018-12-13T17:14:02Z

From the nix gitter from me:
Is it ok to send packets through the loopback interface with sendmsg()/recvmsg() in a test (test/sys/test_socket.rs)?

@asomers replied:
@pusateri it's ok as long as you can guarantee that they won't interfere with anything else
Use the appropriate mutex from test/test.rs or create a new one to protect access to the loopback interface.
And don't assume that its address will be 127.0.0.1.

So that's what I did. Also Linux uses "lo" and BSDs and macOS use "lo0". Rather than hard coding it, it's safer to look for the LOOPBACK flag.

So now which way do you prefer?

asomers · 2018-12-13T17:21:07Z

From the nix gitter from me:
Is it ok to send packets through the loopback interface with sendmsg()/recvmsg() in a test (test/sys/test_socket.rs)?

@asomers replied:
@pusateri it's ok as long as you can guarantee that they won't interfere with anything else
Use the appropriate mutex from test/test.rs or create a new one to protect access to the loopback interface.
And don't assume that its address will be 127.0.0.1.

So that's what I did. Also Linux uses "lo" and BSDs and macOS use "lo0". Rather than hard coding it, it's safer to look for the LOOPBACK flag.

So now which way do you prefer?

Well, that answers the question about hardcoding the interface name and address. I guess it's not an issue for most tests, because most tests only need the address, not the name. As for the mutex, I think it's better to use separate port numbers. Using distinct ports is a way to guarantee that the tests won't interfere with each other.

pusateri · 2018-12-13T17:25:09Z

It already has the kernel assign a random unused port number so all I should need to do is remove the mutex, fix the CHANGELOG, and remove the unnecessary u8's?

asomers · 2018-12-13T17:29:00Z

Sounds good.

asomers · 2018-12-13T18:34:15Z

CHANGELOG.md

@@ -6,7 +6,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]
 - Added PKTINFO cmsg support on Android/iOS/Linux/MacOS.
 - Added V6PKTINFO cmsg support on Android/FreeBSD/iOS/Linux/MacOS.
- ([#990](https://github.com/nix-rust/nix/pull/990))
+  ([#990](https://github.com/nix-rust/nix/pull/990))


This still isn't correct because the link is only associated with the V6PKTINFO bullet. You need to either duplicate the link for both bullets, or collapse everything into a single bullet.

asomers · 2018-12-15T17:37:57Z

Ok, everything is good! Please squash and then I'll merge.

@mcginty

ignore pktinfo tests on qemu mips,mips64,powerpc64 Original work by @mcginty.

asomers

bors r+

990: Add cmsg PKTINFO for IPv4 and IPv6. r=asomers a=pusateri Replaces #891 and attempts to address all previous concerns. Co-authored-by: Tom Pusateri <[email protected]>

bors · 2018-12-17T05:09:49Z

Build succeeded

pusateri mentioned this pull request Dec 4, 2018

support PKTINFO cmsgs for Linux/macOS/iOS #891

Closed

asomers reviewed Dec 4, 2018

View reviewed changes

pusateri mentioned this pull request Dec 10, 2018

Incoming interface would be useful for multicast UDP rust-lang/socket2#29

Closed

asomers reviewed Dec 13, 2018

View reviewed changes

Add cmsg PKTINFO for IPv4 and IPv6.

940a8de

ignore pktinfo tests on qemu mips,mips64,powerpc64 Original work by @mcginty.

asomers approved these changes Dec 17, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Dec 17, 2018

Merge #990

f404d9d

990: Add cmsg PKTINFO for IPv4 and IPv6. r=asomers a=pusateri Replaces #891 and attempts to address all previous concerns. Co-authored-by: Tom Pusateri <[email protected]>

bors bot merged commit 940a8de into nix-rust:master Dec 17, 2018

pusateri mentioned this pull request Dec 24, 2018

IPV6_RECVPKTINFO and IP_PKTINFO support #890

Closed

pusateri mentioned this pull request Jan 4, 2019

Consider supporting advanced networking APIs tokio-rs/tokio#799

Closed

bltavares mentioned this pull request Jun 26, 2020

Discovery: MDNS does not find devices across multiple interfaces bltavares/colmeia#15

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cmsg PKTINFO for IPv4 and IPv6. #990

Add cmsg PKTINFO for IPv4 and IPv6. #990

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

asomers Dec 4, 2018

asomers Dec 4, 2018

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

asomers commented Dec 5, 2018

pusateri commented Dec 5, 2018

asomers commented Dec 5, 2018

pusateri commented Dec 5, 2018

pusateri commented Dec 6, 2018

pusateri commented Dec 11, 2018

pusateri commented Dec 11, 2018

asomers commented Dec 11, 2018

pusateri commented Dec 11, 2018

asomers commented Dec 11, 2018

asomers commented Dec 12, 2018

asomers Dec 13, 2018

asomers Dec 13, 2018

asomers Dec 13, 2018

asomers Dec 13, 2018

pusateri Dec 13, 2018

pusateri commented Dec 13, 2018

asomers commented Dec 13, 2018

pusateri commented Dec 13, 2018

asomers commented Dec 13, 2018

asomers Dec 13, 2018

asomers commented Dec 15, 2018

asomers left a comment

bors bot commented Dec 17, 2018

Add cmsg PKTINFO for IPv4 and IPv6. #990

Add cmsg PKTINFO for IPv4 and IPv6. #990

Conversation

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pusateri commented Dec 4, 2018

pusateri commented Dec 4, 2018

asomers commented Dec 5, 2018

pusateri commented Dec 5, 2018

asomers commented Dec 5, 2018

pusateri commented Dec 5, 2018

pusateri commented Dec 6, 2018

pusateri commented Dec 11, 2018

pusateri commented Dec 11, 2018

asomers commented Dec 11, 2018

pusateri commented Dec 11, 2018

asomers commented Dec 11, 2018

asomers commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pusateri commented Dec 13, 2018

asomers commented Dec 13, 2018

pusateri commented Dec 13, 2018

asomers commented Dec 13, 2018

Choose a reason for hiding this comment

asomers commented Dec 15, 2018

asomers left a comment

Choose a reason for hiding this comment

bors bot commented Dec 17, 2018

Build succeeded