Skip to content

Conversation

@cglosner
Copy link

The extended headers are for echo and reply echo messages.

cglosner added 2 commits July 18, 2023 13:21
Added descriptions for structures and constants for ipv4 icmp echo and reply extended headers.
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #4045 (1ad5923) into master (022df2b) will increase coverage by 0.0%.
The diff coverage is n/a.

see 1 file with indirect coverage changes

ident icmp_ext_echo_iio_ident
} [packed]

icmp_ext_echo_iio_ident {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it rather be a union? I.e. either name, or ipv4, or ipv6?

id int16be
seq_num int16be
icmp_ext_hdr icmp_ext_hdr
iio icmp_ext_echo_iio
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://datatracker.ietf.org/doc/html/draft-bonica-intarea-eping-04#section-2

When applied to the ICMP  Extended Echo Request message, the ICMP Extension Structure contains 
one or two instances of the Interface Identification Object

seq_num int16be
icmp_ext_hdr icmp_ext_hdr
iio icmp_ext_echo_iio
data array[int8]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the packet include any arbitrary payload beyond what's described above?

The question is related to both request and reply structures.

id int16be
seq_num int16be
icmp_ext_hdr icmp_ext_hdr
iio icmp_ext_echo_iio
Copy link
Collaborator

Choose a reason for hiding this comment

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

@a-nogikh
Copy link
Collaborator

Please also address this CI error:

Error: Wrong commit subject format: 'AUTHORS/CONTRIBUTORS: Added name'. Please use 'main/affected/package: short change description'. See docs/contributing.md for details.

It's about your first commit's description.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants