-
Notifications
You must be signed in to change notification settings - Fork 446
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
Implement set_ttl for Tcp and Udp sockets #59
Conversation
@@ -270,7 +270,8 @@ mod test { | |||
src_addr: Ipv4Address([10, 0, 0, 1]), | |||
dst_addr: Ipv4Address([10, 0, 0, 2]), | |||
protocol: IpProtocol::Unknown(IP_PROTO), | |||
payload_len: 4 | |||
payload_len: 4, | |||
ttl: 64 |
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 isn't ergonomic to have to specify the ttl
member on initialization.
assert_eq!(Repr::parse(&packet, &ChecksumCapabilities::default()), Err(Error::Malformed)); | ||
} | ||
|
||
#[test] |
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 totally unrelated to the TTL value change. I'm 100% fine with backing this out.
src/wire/ip.rs
Outdated
} | ||
|
||
/// Set the TTL value. | ||
pub fn set_ttl(&mut self, limit: u8) { |
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 isn't super useful. I'd be okay with backing this 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.
Let's leave this out, set_payload_len
is special 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.
Just a few doc-comment changes needed.
src/socket/tcp.rs
Outdated
self.ttl | ||
} | ||
|
||
/// Set the ttl value. |
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.
Set the time-to-live (IPv4) or hop limit (IPv6) value used in outgoing packets.
src/socket/tcp.rs
Outdated
|
||
/// Set the ttl value. | ||
/// | ||
/// A socket without an explicitly set ttl will fallback to the default [INNA recommended] |
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.
an explicitly set TTL value
, IANA recommended
src/socket/udp.rs
Outdated
self.ttl | ||
} | ||
|
||
/// Set the ttl value. |
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.
Same as above.
src/socket/tcp.rs
Outdated
@@ -179,6 +179,8 @@ pub struct TcpSocket<'a> { | |||
timeout: Option<u64>, | |||
/// Interval at which keep-alive packets will be sent. | |||
keep_alive: Option<u64>, | |||
/// The Hop Limit (ipv6)/TTL (ipv4) to be used for the socket |
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 time-to-live (IPv4) or hop limit (IPv6) value used in outgoing packets.
7168dee
to
723226c
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.
Thanks for the review. Made some updates today.
src/socket/tcp.rs
Outdated
match ttl { | ||
Some(0) => {}, | ||
catchall => self.ttl = catchall, | ||
} |
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.
Realized it was possible to set the TTL value for sent packets to the invalid value of 0 with set_ttl
. Added logic to ensure that the sockets configured TTL value is only changed when the given argument is None
or Some(x)
where x > 0
src/socket/udp.rs
Outdated
/// | ||
/// * A socket without an explicitly set TTL value uses the default [INNA recommended] | ||
/// value (`64`). | ||
/// * A TTL value of 0 is invalid and will be ignored. See [RFC 1122 § 3.2.1.7]. |
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.
Added documentation about the added logic and updated documentation about the default value.
src/socket/udp.rs
Outdated
|
||
// Ensure you cannot set the TTL value to an invalid value. | ||
s.set_ttl(Some(0)); | ||
assert_eq!(s.ttl(), None); |
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.
Added tests for the added logic.
src/socket/tcp.rs
Outdated
|
||
/// Set the time-to-live (IPv4) or hop limit (IPv6) value used in outgoing packets. | ||
/// | ||
/// * A socket without an explicitly set TTL value uses the default [INNA recommended] |
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.
INNA → IANA
src/socket/tcp.rs
Outdated
/// | ||
/// * A socket without an explicitly set TTL value uses the default [INNA recommended] | ||
/// value (`64`). | ||
/// * A TTL value of 0 is invalid and will be ignored. See [RFC 1122 § 3.2.1.7]. |
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.
TTL value of 0 is clearly user error so you should panic rather than silently ignore.
self.ttl | ||
} | ||
|
||
/// Set the time-to-live (IPv4) or hop limit (IPv6) value used in outgoing packets. |
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.
Same as above.
src/wire/ip.rs
Outdated
} | ||
|
||
/// Set the TTL value. | ||
pub fn set_ttl(&mut self, limit: u8) { |
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.
Let's leave this out, set_payload_len
is special here.
Good catches. Updated. |
- Add the ttl member to the IpRepr - Add the ttl member along with setters and getters to the tcp and udp socket types - Add unit tests for the new set_ttl parameter - Update usage of IpRepr to include the ttl value
c9c6efe
to
73c8be9
Compare
Removed |
Sorry for the delay. |
No problem |
ttl
member to theIpRepr
ttl
member along with setters and getters to the tcp and udpsocket types
set_ttl
parameterIpRepr
to include thettl
valueResolves: #58