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.

Sign up for GitHub

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

Jump to bottom

Implement ICMP #38

Closed
wants to merge 5 commits into from
Closed

Implement ICMP #38

wants to merge 5 commits into from

Conversation

hgupta12
Copy link
Contributor

This PR adds ICMP packet dissection.
It also takes care of the compilation issue on Windows.

Fixes #29

@hgupta12
[ICMP] Implement ICMP Header dissection
600894f 
Signed-off-by: harshit <harshitgupta1406@gmail.com>
gabhijit
gabhijit requested changes Feb 22, 2024
View reviewed changes
Copy link
Collaborator

@gabhijit gabhijit left a comment

Choose a reason for hiding this comment

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

TODO:

  1. couple of nit picks
  2. 2few suggestions to support better parsing for EchoRequest and EchoReply.
  3. Adding test cases for supported types and one unspported type (any).

src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Show resolved Hide resolved
src/layers/icmp.rs Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
@hgupta12
[ICMP] Replace type values with constants
88bac36 
Signed-off-by: harshit <harshitgupta1406@gmail.com>
gabhijit
gabhijit reviewed Feb 23, 2024
View reviewed changes
Copy link
Collaborator

@gabhijit gabhijit left a comment

Choose a reason for hiding this comment

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

A couple of high level comments -

  1. The unused in the RFC means those bytes are not used by the ICMP messages. In the resultant structure we can simply ignore those bytes.
  2. Unsupported is different.
  3. Also checksum should be u16 only it's serde formatting needs to be changed.

src/layers/icmp.rs Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
src/layers/icmp.rs Show resolved Hide resolved
src/layers/icmp.rs Outdated Show resolved Hide resolved
@hgupta12
[ICMP] Capture bytes for unsupported types and minor changes
f298932 
Signed-off-by: harshit <harshitgupta1406@gmail.com>
@hgupta12
[ICMP] Add tests
a796fda 
Signed-off-by: harshit <harshitgupta1406@gmail.com>
@hgupta12
[ICMP] Add test for different types
2be23e6 
Signed-off-by: harshit <harshitgupta1406@gmail.com>
@gabhijit gabhijit changed the title Draft: Implement ICMP Implement ICMP Feb 25, 2024
@gabhijit
Copy link
Collaborator

@hgupta12 This is now merged into the master!! Thank you for your contribution!

@gabhijit gabhijit closed this Feb 25, 2024
@gabhijit gabhijit mentioned this pull request Feb 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabhijit gabhijit gabhijit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for dissector for ICMP Packets
2 participants
@hgupta12 @gabhijit