Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

icmpv4 time exceeded message #102

Merged
merged 2 commits into from
May 22, 2020
Merged

Conversation

zeeshanlakhani
Copy link
Member

Description

Time Exceeded Message defined in [IETF RFC 792] (for ICMPv4).

Also includes:

  • doc fixes for links to private fns

Type of change

  • New protocol
  • Documentation update

Checklist

  • Associated tests
  • Associated documentation

Also includes:

- doc fixes for links to private fns
@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #102 into master will increase coverage by 0.14%.
The diff coverage is 73.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   67.00%   67.15%   +0.14%     
==========================================
  Files          63       64       +1     
  Lines        5377     5459      +82     
==========================================
+ Hits         3603     3666      +63     
- Misses       1774     1793      +19     
Impacted Files Coverage Δ
core/src/packets/icmp/v4/mod.rs 83.33% <0.00%> (+1.03%) ⬆️
core/src/packets/icmp/v6/mod.rs 80.15% <ø> (ø)
core/src/packets/icmp/v6/ndp/redirect.rs 75.60% <ø> (ø)
core/src/packets/icmp/v6/time_exceeded.rs 68.65% <ø> (ø)
core/src/packets/icmp/v6/too_big.rs 70.27% <ø> (ø)
core/src/packets/icmp/v4/time_exceeded.rs 74.07% <74.07%> (ø)
core/src/testils/proptest/strategy.rs 83.02% <0.00%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab5001d...ba9e792. Read the comment docs.

Copy link
Contributor

@drunkirishcoder drunkirishcoder left a comment

Choose a reason for hiding this comment

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

👍 good.

@@ -252,7 +252,7 @@ impl<E: Ipv6Packet> Packet for Icmpv6<E> {
/// * [`checksum`] is computed based on the [`pseudo-header`] and the
/// full packet.
///
/// [`checksum`]: Icmpv6::checksum
/// [`checksum`]: Icmpv6Packet::checksum
Copy link
Contributor

Choose a reason for hiding this comment

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

noticed that functions on Icmpv4 and Icmpv6 have inconsistent visibility scopes. msg_type, code, set_code, checksum, compute_checksum should all be pub on Icmpv6. then don't need to fix these links anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

good. I was going to do this, but wasn't sure why we had the inconsistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep not sure how I missed this, but they need to be public so extending Icmpv6 is possible from outside the crate.

@zeeshanlakhani zeeshanlakhani merged commit b10c0a0 into master May 22, 2020
@zeeshanlakhani zeeshanlakhani deleted the zl/icmpv4-time-exceeded branch May 22, 2020 16:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants