-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Implemented status messages for ntpv5. #1209
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1209 +/- ##
==========================================
- Coverage 85.47% 85.09% -0.39%
==========================================
Files 58 58
Lines 17077 17204 +127
==========================================
+ Hits 14596 14639 +43
- Misses 2481 2565 +84 ☔ View full report in Codecov by Sentry. |
ntp-proto/src/packet/mod.rs
Outdated
} | ||
} | ||
} | ||
|
||
pub fn is_kiss(&self) -> bool { | ||
match self.header { | ||
NtpHeader::V3(header) => header.stratum == 0, | ||
NtpHeader::V4(header) => header.stratum == 0, | ||
#[cfg(feature = "ntpv5")] | ||
// TODO NTPv5 does not have Kiss codes so we pretend everything is always fine |
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 todo no longer seems to apply?
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.
removed the todo
ntp-proto/src/packet/mod.rs
Outdated
NtpHeader::V3(header) => header.reference_id, | ||
NtpHeader::V4(header) => header.reference_id, | ||
#[cfg(feature = "ntpv5")] | ||
// Kiss code in ntpv5 is the first four bytes of the server vookie |
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.
Typo: vookie
-> cookie
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.
fixed
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.
Two small things, but other than that LGTM!
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.
Looks good, is just not DRY in some places.
Note: this is based on the current proposal from us, not yet part of the draft.
4fc8649
to
c77e1e3
Compare
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.
Looks good now!
Note: this is based on the current proposal from us, not yet part of the draft.