Add support for SO_TIMESTAMP #663

Wolvereness · 2017-07-09T03:32:16Z

This was implemented as part of my employment, and I have received permission to submit it upstream under my own name.

I implemented this with a bit of copy+pasting from the SCM_RIGHTS implementation, and it appeared to be functional when originally implemented on top of 13deb61 (tagged v0.8.0).

Susurrus · 2017-07-09T03:50:27Z

src/sys/socket/consts.rs

@@ -109,6 +109,7 @@ mod os {

    // Ancillary message types
    pub const SCM_RIGHTS: c_int = libc::SCM_RIGHTS;
+    pub const SCM_TIMESTAMP: c_int = SO_TIMESTAMP;


This constant is declared in libc, so it should reference that constant instead of defining it directly.

Susurrus · 2017-07-09T03:50:33Z

src/sys/socket/consts.rs

@@ -235,6 +236,7 @@ mod os {

    // Ancillary message types
    pub const SCM_RIGHTS: c_int = 1;
+    pub const SCM_TIMESTAMP: c_int = SO_TIMESTAMP;


Same as above.

Susurrus · 2017-07-09T03:51:02Z

src/sys/socket/mod.rs

@@ -182,6 +187,7 @@ pub enum ControlMessage<'a> {
    /// "Ancillary messages" section of the
    /// [unix(7) man page](http://man7.org/linux/man-pages/man7/unix.7.html).
    ScmRights(&'a [RawFd]),
+    ScmTimestamp(&'a TimeVal),


This needs a doc comment.

Susurrus · 2017-07-09T03:52:30Z

Awesome, thanks for your contribution! I've added a few minor comments. Please also add a changelog entry under the Added section.

Wolvereness · 2017-07-09T04:43:18Z

Added changelog entry, documentation blurb, and used the SCM_TIMESTAMP declaration.

However, that appears to have broken the build; SCM_TIMESTAMP is at minimum inconsistently declared.

Here's a breakdown of where libc::SCM_TIMESTAMP is defined, based on a travis-ci build:

STATUS	TARGET
❌	aarch64-linux-android
❌	arm-linux-androideabi
❌	armv7-linux-androideabi
❌	i686-linux-android
❌	x86_64-linux-android
✔️	aarch64-unknown-linux-gnu
❌	arm-unknown-linux-gnueabi
❌	armv7-unknown-linux-gnueabihf
❌	i686-unknown-linux-gnu
❌	i686-unknown-linux-musl
✔️	mips-unknown-linux-gnu
✔️	mipsel-unknown-linux-gnu
❌	powerpc-unknown-linux-gnu
✔️	powerpc64-unknown-linux-gnu
✔️	powerpc64le-unknown-linux-gnu
✔️	x86_64-unknown-linux-gnu
❌	x86_64-unknown-linux-musl
✔️	i686-apple-darwin
✔️	x86_64-apple-darwin
❌	i686-unknown-freebsd
✔️	x86_64-unknown-netbsd
✔️	x86_64-unknown-linux-gnu
✔️	x86_64-apple-darwin

It appears that for many of these, it might be a bug for libc's crate for not defining them. I can't confirm any target where it shouldn't be found; every c-header researched has had it defined. Some of them do not alias it (or share the value) as SO_TIMESTAMP though, so assigning it to libc::SO_TIMESTAMP will fail on those targets.

However, I can say that I can't personally pursue fixing this upstream, other than opening an issue: rust-lang/libc#655

ndusart · 2017-07-09T07:10:57Z

I will include it in my PR as I'm trying to add missing sockets constants (rust-lang/libc#656).

Susurrus · 2017-07-09T16:13:43Z

@Wolvereness Yes, most of our issues end up being libc issues unfortunately as that crate is going through puberty and many things are undefined or defined incorrectly. But it's the path of least maintenance for us moving forward so we try to power through it.

Can you change Cargo.toml in this PR to reference the PR @ndusart filed so we can make sure CI will pass for all platforms when that PR lands? We can change Cargo.toml back before we merge this.

Could you please update this PR to use the libc types directly as @ndusart has volunteered to get this constant upstreamed for you. Thanks @ndusart!

Wolvereness · 2017-07-09T18:09:22Z

@ndusart I'm appreciative that you can take that on; thanks!

@Susurrus I'll do that as soon as I'm able, which I'd expect in the next day or so.

Wolvereness · 2017-07-11T07:43:35Z

@Susurrus I reverted to the libc::SCM_TIMESTAMP commit, and added a commit that changes the Cargo.toml libc to point to master in their repo. Based on a preliminary travis-ci, it appears to compile properly now. The upstream commit that matters is rust-lang/libc@472ffbb.

Susurrus · 2017-07-11T16:40:34Z

The whole consts module is going to go away with #647, so instead of adding SCM_TIMESTAMP, just refer to libc directly for it. That way this shouldn't need much of a rebase once #647 lands.

Wolvereness · 2017-07-13T07:22:50Z

@Susurrus Either way one of them will require a merge conflict resolution. If the plan was to include this one sooner, and have #647 rebase, I can go ahead and fix this one up to the new style. But, I'm under the assumption that both are still waiting on libc to hit a release cycle.

The least amount of effort will be fixing this one up to a certain style when it's intended to be merged. Let me know what the plan is, and I can put in the work when stuff is ready.

Wolvereness · 2017-07-26T00:10:04Z

I went ahead and prepared it with the new style of using libc instead of the consts. Upstream has already put the constants into a release cycle, so as long as the version in the toml doesn't get set back down to an earlier release, that wont be an issue either.

asomers · 2017-07-26T02:38:53Z

I'm not familiar with the api, but it looks ok to me. I think the test failure on rust=beta will go away if you rebase. @Susurrus do you want to look too?

Wolvereness · 2017-07-26T10:57:59Z

@asomers The commit is up-to-date with master; there's nothing to rebase other than to retry the build.

There's a problem in travis-ci, as beta should actually be an allowed failure at the moment, but it's using an old one for this PR. All of the current builds are failing in beta, but they are correctly marked as allowed failure.

Susurrus

Some documentation changes are needed. And is there anyway to write tests for this? I'd really like to get some testing this functionality if possible.

Susurrus · 2017-07-26T17:01:58Z

src/sys/socket/mod.rs

@@ -182,6 +188,11 @@ pub enum ControlMessage<'a> {
    /// "Ancillary messages" section of the
    /// [unix(7) man page](http://man7.org/linux/man-pages/man7/unix.7.html).
    ScmRights(&'a [RawFd]),
+    /// A message of type SCM_TIMESTAMP, containing the time the packet


This comment is too long so it looks terrible in the generated docs. You should generate the docs on your end to check (cargo doc --no-deps --open) and search for ScmTimestamp and you'll see what I'm talking about.

All docs should have a short descriptive sentence at the start, followed by an empty comment line, then a more descriptive text. Please see the Rust book for some more details on writing effective documentation.

I can touch that up with the other changes.

Susurrus · 2017-07-26T17:03:08Z

CHANGELOG.md

@@ -5,6 +5,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]

+### Added
+- Added `nix::sys::socket::ControlMessage::ScmTimestamp`


What does this mean for the user? Adding an enum variant, so what? Is there some new functionality we can describe here. If you were someone who wanted to use nix and you saw this, would you know what it meant? I think this needs a little more description here.

I could add nix::sys::socket::sockopt::ReceiveTimestamp was added, otherwise a binding for a particular socket control message should be straightforward to someone looking for the functionality of the same name.

I didn't see any other added changelog entries that added more than the shared name of the new binding, so I'm not sure what level of detail is appropriate.

I don't really understand this subsystem as it's missing pretty much all relevant documentation, but you added a new ControlMessage variant to handle SCM_TIMESTAMP messages. You also added ReceiveTimestamp as something, that I think is user-facing, but I don't understand what it is and how it's used, but it should probably be documented under "Added" as well if it's a public API addition.

Wolvereness · 2017-07-27T23:37:52Z

@Susurrus Technically speaking, testing the availability of UDP packets isn't deterministic. From a practical perspective, I'm not prepared to write tests for it, nor am I even sure it's supported on all targets (though they do have the constants apparently).

Susurrus · 2017-08-03T03:53:49Z

@Wolvereness This needs a rebase and some clarify for the CHANGELOG. Also some tests are failing which should be investigated.

Wolvereness · 2017-08-08T07:32:26Z

Added a test. Two UDP packets are sent to the same socket, 250 ms apart. The test fails if timestamps are >10ms apart from expected delay, there are no timestamps, the packets are incorrect, or not both packets were received.

Wolvereness · 2017-08-09T22:43:30Z

@Susurrus The x86-freebsd (at least on buildbot) doesn't seem to give back the control messages, so I've disabled the API for that target. I also changed the measurement to compare the difference between the time taken within 5ms accuracy (which, from observation, it tends to be abs((sent2 - sent1) - (received2 - received1)) <40μs). That appears to have fixed the x64-freebsd system.

asomers · 2017-08-09T23:07:45Z

Before we accept this we need to figure out why your test fails on i386-freebsd. Simply disabling the API on that architecture is probably not correct.

Wolvereness · 2017-08-09T23:28:45Z

@asomers It's a tier-2 target. I don't have access to a machine running that target either, other than spamming PRs to see what sticks on buildbot.

A volunteer could run it an look at what CmsgSpace ends up containing; maybe it's a mismatched constant. If there's nothing, even with an excessively sized buffer, then that's an issue for the developers of that C-API implementation.

asomers · 2017-08-09T23:32:47Z

@Wolvereness I'm just such a volunteer. I'll try it out tonight.

asomers · 2017-08-10T16:02:30Z

I grabbed @Wolvereness's branch, removed the restrictions for FreeBSD x86, and the tests all passed in my 32 bit VM. Next I need to try to replicate the Buildbot's setup. That system is actually a 32-bit userland with a 64-bit kernel.

Susurrus

Just a couple of last doc comment edits, then I think I'm done here.

Susurrus · 2017-08-11T15:48:54Z

src/sys/socket/mod.rs

    /// [unix(7) man page](http://man7.org/linux/man-pages/man7/unix.7.html).
    ScmRights(&'a [RawFd]),
+    /// A message of type SCM_TIMESTAMP, containing the time the packet


Backticks around "SO_TIMESTAMP" here and on 290.

The later usage of "SO_TIMESTAMP" is not referring to the identifier, it's referring to the section name, as is the earlier "Ancillary messages".

Susurrus · 2017-08-11T15:49:16Z

src/sys/socket/mod.rs

@@ -274,10 +279,77 @@ impl<'a> Iterator for CmsgIterator<'a> {
 /// [Further reading](http://man7.org/linux/man-pages/man3/cmsg.3.html)
 pub enum ControlMessage<'a> {
    /// A message of type SCM_RIGHTS, containing an array of file


Since you're here, can you also put SCM_RIGHTS in backticks?

Wolvereness · 2017-08-13T03:38:41Z

@Susurrus I can take care of that just after @asomers has an update on the FreeBSD target.

Susurrus · 2017-09-01T02:42:10Z

@asomers Any update on the FreeBSD target issues?

asomers · 2017-09-01T14:35:59Z

I'll try to do the 32-bit on 64-bit test this weekend.

asomers · 2017-09-04T03:14:07Z

I've tracked it down to a FreeBSD kernel bug. Only 32-bit emulation is affected. The test works fine with a native i386 kernel. So you should enable all relevant functionality for FreeBSD i386, but disable the test like this. We will reenable the test whenever I update Buildbot to a version that fixes this bug.

    /// # Examples
    ///
    // Disable this test on FreeBSD i386
    // https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=222039
#[cfg_attr(not(all(target_os = "freebsd", target_arch = "x86")), doc = " ```")]
#[cfg_attr(all(target_os = "freebsd", target_arch = "x86"), doc = " ```no_run")]
    /// use nix::sys::socket::*;
    /// use nix::sys::uio::IoVec;
    /// use nix::sys::time::*;
    /// use std::time::*;

Wolvereness · 2017-09-04T05:42:32Z

@asomers That's an unbelievably nifty trick for having a rust doc test excluded for certain targets.

And, I'm trying to throw in keywords to this message in hopes it comes up in a search for someone in the future, as I couldn't find how to do that myself.

asomers · 2017-09-07T04:36:43Z

FYI the bug is now fixed. But we must still leave that test disabled for now, because I'm not going to update the worker's image until 11.2 is released.
https://svnweb.freebsd.org/base?view=revision&revision=323254

bors r+

663: Add support for SO_TIMESTAMP r=asomers a=Wolvereness This was implemented as part of my employment, and I have received permission to submit it upstream under my own name. I implemented this with a bit of copy+pasting from the SCM_RIGHTS implementation, and it appeared to be functional when originally implemented on top of 13deb61 (tagged v0.8.0).

bors · 2017-09-07T06:59:33Z

Build succeeded

1402: Support TIMESTAMPNS r=asomers a=WiSaGaN This adds support of linux TIMESTAMPNS. The code is mostly copied paste from #663 Co-authored-by: Lu, Wangshan <wisagan@gmail.com>

Susurrus suggested changes Jul 9, 2017

View reviewed changes

Wolvereness force-pushed the SO_TIMESTAMP branch from 3a75bfa to 107d8af Compare July 9, 2017 04:38

Wolvereness force-pushed the SO_TIMESTAMP branch from 107d8af to a2608c9 Compare July 9, 2017 04:46

Wolvereness force-pushed the SO_TIMESTAMP branch from a2608c9 to 7b21417 Compare July 11, 2017 07:31

Wolvereness force-pushed the SO_TIMESTAMP branch from 7b21417 to 3c1811a Compare July 26, 2017 00:00

Susurrus suggested changes Jul 26, 2017

View reviewed changes

Wolvereness force-pushed the SO_TIMESTAMP branch from 3c1811a to 746309c Compare August 8, 2017 07:23

Wolvereness force-pushed the SO_TIMESTAMP branch 7 times, most recently from 54d4a37 to 8a7c977 Compare August 9, 2017 22:30

Susurrus suggested changes Aug 11, 2017

View reviewed changes

Add support for SO_TIMESTAMP

dba1bb7

Wolvereness force-pushed the SO_TIMESTAMP branch from 8a7c977 to dba1bb7 Compare September 4, 2017 05:22

bors bot merged commit dba1bb7 into nix-rust:master Sep 7, 2017

asomers mentioned this pull request Sep 28, 2017

CI for FreeBSD tokio-rs/mio#738

Closed

WiSaGaN mentioned this pull request Mar 20, 2021

Support TIMESTAMPNS #1402

Merged

bors bot added a commit that referenced this pull request Apr 8, 2021

Merge #1402

68d01f0

1402: Support TIMESTAMPNS r=asomers a=WiSaGaN This adds support of linux TIMESTAMPNS. The code is mostly copied paste from #663 Co-authored-by: Lu, Wangshan <wisagan@gmail.com>

Add support for SO_TIMESTAMP #663

Add support for SO_TIMESTAMP #663

Conversation

Wolvereness commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 9, 2017

Wolvereness commented Jul 9, 2017 • edited Loading

ndusart commented Jul 9, 2017

Susurrus commented Jul 9, 2017

Wolvereness commented Jul 9, 2017

Wolvereness commented Jul 11, 2017

Susurrus commented Jul 11, 2017

Wolvereness commented Jul 13, 2017

Wolvereness commented Jul 26, 2017

asomers commented Jul 26, 2017

Wolvereness commented Jul 26, 2017 • edited Loading

Susurrus left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wolvereness commented Jul 27, 2017

Susurrus commented Aug 3, 2017

Wolvereness commented Aug 8, 2017 • edited Loading

Wolvereness commented Aug 9, 2017 • edited Loading

asomers commented Aug 9, 2017

Wolvereness commented Aug 9, 2017

asomers commented Aug 9, 2017

asomers commented Aug 10, 2017

Susurrus left a 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

Wolvereness commented Aug 13, 2017

Susurrus commented Sep 1, 2017

asomers commented Sep 1, 2017

asomers commented Sep 4, 2017

Wolvereness commented Sep 4, 2017

asomers commented Sep 7, 2017

bors bot commented Sep 7, 2017

Build succeeded

Wolvereness commented Jul 9, 2017 •

edited

Loading

Wolvereness commented Jul 26, 2017 •

edited

Loading

Wolvereness commented Aug 8, 2017 •

edited

Loading

Wolvereness commented Aug 9, 2017 •

edited

Loading