-
Notifications
You must be signed in to change notification settings - Fork 667
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 IP_RECVIF & IP_RECVDSTADDR. #1002
Conversation
Missing a constant in libc. Will be ready when libc pull request is merged rust-lang/libc#1189 |
Ok, libc change has been merged. Not sure how to re-run the CI tests but no code changes should be needed. |
I restarted Travis manually. Should've only done the one target, but I didn't realize it was only 1 until too late. |
I'm afraid you'll need to rebase, due to CI changes. |
Just tried this myself and I can't get the doc test to fail on FreeBSD 11.2 amd64. I notice there's a cfg option to turn this doc test off for i386. What's that all about?
|
There may be a problem with the test environment. |
The test is not really valid. It's basically asserting that the sleep plus the 2nd |
.intersects(MsgFlags::MSG_TRUNC | MsgFlags::MSG_CTRUNC) | ||
); | ||
|
||
for cmsg in msg.cmsgs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing to ensure that cmsgs were actually received. Shouldn't you have some kind of assertion to that effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your latest updated didn't address this issue. Do you plan to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you just want something like this:
assert_eq!(msg.cmsgs().count(), 2, "expected 2 cmsgs");
or did you have something different in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That statement wouldn't check that you got the right types of cmsgs. I think you need to check that the cmsgs were of the right types, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the match condition '_' in the for loop would assert if it was any other type. So this would ensure there were two messages and the types were either Ipv4RecvIf or Ipv4RecvDstAddr. Since the kernel can't send two of the same type, I think this is sufficient, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test should verify that we got one of each. Otherwise it wouldn't catch a regression that decodes the types incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just added some code that I think covers this. So now, I check to make sure they're are only two and I check to make sure each type is received and it fails if something other than the expected two message types are received.
test/sys/test_socket.rs
Outdated
).expect("send socket failed"); | ||
sendmsg(send, &iov, &[], MsgFlags::empty(), Some(&sa)).expect("sendmsg failed"); | ||
|
||
thread.join().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need a separate thread. sendmsg(2) should return before recvmsg(2) gets called.
test/sys/test_socket.rs
Outdated
use std::io::Write; | ||
use std::thread; | ||
|
||
fn loopback_v4addr() -> Option<InterfaceAddress> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting deja vu here. Didn't a recent PR of yours contain something similar or identical to this function? If so, could you make it common code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this is looking good now! It just needs a CHANGELOG entry and a squash.
Include IP_PKTINFO and IP6_PKTINFO on netbsd/openbsd.
Anything else I need to do here? |
I've restarted the Cirrus test. We'll see if it actually runs this time. |
Still is still ready to merge and the CI is still failing for no good reason. I don't think the FreeBSD CI change is working well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the third time was the charm. I've opened an issue upstream with Cirrus; we'll see what they say.
bors r+
1002: Add IP_RECVIF & IP_RECVDSTADDR. r=asomers a=pusateri Add IP_RECVIF & IP_RECVDSTADDR on freebsd, ios, macos, netbsd, openbsd Include IP_PKTINFO on netbsd Include IP6_PKTINFO on netbsd, openbsd. FreeBSD/OpenBSD do not support IP_PKTINFO for IPv4 but use IP_RECVIF for interface index and use IP_RECVDSTADDR for destination address. NetBSD and macOS also support IP_RECVIF and IP_RECVDSTADDR in addition to IP_PKTINFO for IPv4. (For IPv6, all use IPV6_PKTINFO) Co-authored-by: Tom Pusateri <[email protected]>
Build failed
|
bors retry |
1002: Add IP_RECVIF & IP_RECVDSTADDR. r=asomers a=pusateri Add IP_RECVIF & IP_RECVDSTADDR on freebsd, ios, macos, netbsd, openbsd Include IP_PKTINFO on netbsd Include IP6_PKTINFO on netbsd, openbsd. FreeBSD/OpenBSD do not support IP_PKTINFO for IPv4 but use IP_RECVIF for interface index and use IP_RECVDSTADDR for destination address. NetBSD and macOS also support IP_RECVIF and IP_RECVDSTADDR in addition to IP_PKTINFO for IPv4. (For IPv6, all use IPV6_PKTINFO) Co-authored-by: Tom Pusateri <[email protected]>
Build succeeded
|
FWIW The Cirrus people say they've fixed the bug now. |
Great. Thanks! False negatives waste a lot of time. |
This was an oversight from PR nix-rust#1002
This was an oversight from PR nix-rust#1002
This was an oversight from PR nix-rust#1002
This was an oversight from PR nix-rust#1002
This was an oversight from PR nix-rust#1002
This was an oversight from PR nix-rust#1002
Add IP_RECVIF & IP_RECVDSTADDR on freebsd, ios, macos, netbsd, openbsd
Include IP_PKTINFO on netbsd
Include IP6_PKTINFO on netbsd, openbsd.
FreeBSD/OpenBSD do not support IP_PKTINFO for IPv4 but use IP_RECVIF for interface index and use IP_RECVDSTADDR for destination address.
NetBSD and macOS also support IP_RECVIF and IP_RECVDSTADDR in addition to IP_PKTINFO for IPv4.
(For IPv6, all use IPV6_PKTINFO)