Skip to content

Comments

ICMP handling#308

Closed
rrobgill wants to merge 1 commit intoGyulyVGC:mainfrom
rrobgill:icmp
Closed

ICMP handling#308
rrobgill wants to merge 1 commit intoGyulyVGC:mainfrom
rrobgill:icmp

Conversation

@rrobgill
Copy link

@rrobgill rrobgill commented Jul 15, 2023

Identifies ICMP v4 or v6 packets, and collapses them to a single ICMP transport type.
Ports are undefined by ICMP, therefore to zero.
Protocol type set to ICMP to allow filtering.
Issue #288

@rrobgill
ICMP handling
8660355 
Identifies ICMPv6 or ICMPv4 packets, and collapses them to a single ICMP transport type.
Ports are undefined by ICMP, therefore to zero. Protocol type set to ICMP to allow filtering.

Signed-off-by: Rob Gill <rrobgill@protonmail.com>
@GyulyVGC GyulyVGC added the enhancement New feature, request, or improvement label Jul 15, 2023
@GyulyVGC GyulyVGC added this to the v1.3.0 milestone Jul 15, 2023
@GyulyVGC GyulyVGC linked an issue Jul 15, 2023 that may be closed by this pull request
Support for ICMP #288
Closed
@GyulyVGC
Copy link
Owner

GyulyVGC commented Jul 15, 2023

Thanks so much!
The PR could remain open for a bit, since at least one other release will be published before v1.3.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Aug 22, 2023

Some comments:

  • It's better to not categorise ICMP as transport/application protocol. In theory, it belongs to the network layer but I wouldn't consider it a network protocol as well.
  • After having thought about it, it's better to consider ICMP packets separately from the others.
  • It'd be nice to also show the kind of message of each ICMP packet.

These ideas obviously aren't compatible with the current structure of the app, therefor major changes have to be made in this direction.

Of course, I'll personally take care of the implementation after having figured out what could work to manage ICMP.
I wanted to hear your ideas about these concerns.

@GyulyVGC GyulyVGC marked this pull request as draft September 9, 2023 09:37
@GyulyVGC
Copy link
Owner

GyulyVGC commented Dec 20, 2023

Closing this PR in favour of a more complete implementation provided in #417

@GyulyVGC GyulyVGC closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement New feature, request, or improvement

Projects

None yet

Milestone

v1.3.0

Development

Successfully merging this pull request may close these issues.

Support for ICMP

2 participants

@rrobgill @GyulyVGC