-
Notifications
You must be signed in to change notification settings - Fork 654
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
Support linux TIMESTAMPING #1420
Conversation
@@ -562,6 +562,11 @@ pub enum ControlMessageOwned { | |||
/// [Further reading](https://www.kernel.org/doc/html/latest/networking/timestamping.html) | |||
#[cfg(all(target_os = "linux"))] | |||
ScmTimestampns(TimeSpec), | |||
/// Configurable nanoseconds resolution timestamps |
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.
Could you please explain what the wrapped type is? Why are there three timespecs?
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 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.
A link to an external website is not sufficient documentation. Why is the wrapped value an array? What's special about "3"?
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 link was specifically for your question. Have you checked it out?
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.
No; I'm asking for Nix's own documentation to describe why the value is a 3-member array. You can leave the external link in there, similar to how many Nix functions have man page links, but Nix's own documentation should have at least basic information about what the type is.
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. Added some documentation.
@@ -563,6 +563,8 @@ pub enum ControlMessageOwned { | |||
#[cfg(all(target_os = "linux"))] | |||
ScmTimestampns(TimeSpec), | |||
/// Configurable nanoseconds resolution timestamps | |||
/// Three `TimeSpec`s are returned in the control message. At least one field is non-zero at | |||
/// any time. Most timestamps are passed in ts[0]. Hardware timestamps are passed in ts[2]. |
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.
So what is ts[1] for? Also, the second line of a multiline doc comment like this should be blank. That way, rustdoc will interpret the first line as a summary and the rest as extended details.
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 it is necessary to specify every nuances in this short description. User of this feature needs to read the original linux documentation to have a reasonable idea in how to set and what to set in order to get either software and hardware timestamp (in hardware case, user also need to initiate in different way according to different hardware), and to know the historical reason for ts[1]
being deprecated. I don't see the point to just copy large paragraphs of the original text here.
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.
ts[1] is deprecated? In that case, there isn't any reason to expose it to the user at all. You should remove it in ControlMessageOwned::decode_from
.
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.
This is a pretty highly specific interface. It is more likely the user has learned the original linux interface first and try to use it through nix
crate than people directly learn it through nix
itself. Thus, preserving the original interface as much as possible (close to 1-1 map) can reduce the amount of surprise and avoid possible footguns.
703089e
to
c9ccd9a
Compare
Hi! Was there any particular reason this pull request was closed? I was looking forward to using the timestamping-interface to get access to outgoing timestamps. Would be glad to help with any outstanding problems! (we're a team of 3 Devs, but kinda new to Rust) |
@FloLo100 I don't see this pull request getting merged any time soon. But feel free to reuse/copy any of the code here if you think they are useful. |
@asomers is there anything beyond the documentation that's blocking the merge? We will try to fork this and merge it locally for now. |
Similar to #1402, while this one takes a
u32
for flags, and gets 3Timespec
as a result.