Skip to content
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

Merged
merged 1 commit into from
Oct 24, 2017

Conversation

dlrobertson
Copy link
Collaborator

  • 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

Resolves: #58

@@ -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
Copy link
Collaborator Author

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]
Copy link
Collaborator Author

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.

Sorry, something went wrong.

src/wire/ip.rs Outdated
}

/// Set the TTL value.
pub fn set_ttl(&mut self, limit: u8) {
Copy link
Collaborator Author

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.

Sorry, something went wrong.

Copy link
Contributor

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.

Copy link
Contributor

@whitequark whitequark left a 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.

self.ttl
}

/// Set the ttl value.
Copy link
Contributor

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.


/// Set the ttl value.
///
/// A socket without an explicitly set ttl will fallback to the default [INNA recommended]
Copy link
Contributor

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

self.ttl
}

/// Set the ttl value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@@ -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
Copy link
Contributor

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.

@dlrobertson dlrobertson force-pushed the add_ttl branch 2 times, most recently from 7168dee to 723226c Compare October 16, 2017 00:51
Copy link
Collaborator Author

@dlrobertson dlrobertson left a 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.

match ttl {
Some(0) => {},
catchall => self.ttl = catchall,
}
Copy link
Collaborator Author

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

///
/// * 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].
Copy link
Collaborator Author

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.


// Ensure you cannot set the TTL value to an invalid value.
s.set_ttl(Some(0));
assert_eq!(s.ttl(), None);
Copy link
Collaborator Author

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.


/// 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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INNA → IANA

///
/// * 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].
Copy link
Contributor

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.
Copy link
Contributor

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) {
Copy link
Contributor

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.

@dlrobertson
Copy link
Collaborator Author

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
@dlrobertson dlrobertson force-pushed the add_ttl branch 2 times, most recently from c9c6efe to 73c8be9 Compare October 16, 2017 23:03
@dlrobertson
Copy link
Collaborator Author

Removed * IPv4 time-to-live value is fixed at 64. from the README

@whitequark
Copy link
Contributor

Sorry for the delay.

@whitequark whitequark merged commit fea4628 into smoltcp-rs:master Oct 24, 2017
@dlrobertson
Copy link
Collaborator Author

No problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants