IGMPv1 header support#142
Merged
JulianSchmid merged 15 commits intoJulianSchmid:masterfrom May 2, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds IGMP (v1/v2/v3) support to the transport layer: new IGMP types, headers, parsing, serialization, checksum calculation, value wrappers, constants, module wiring, error enums, docs, and unit/property tests. ChangesIGMP Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etherparse/src/err/layer.rs`:
- Around line 53-54: Revert the addition of the public enum variant Igmpv1 to
Layer (do not modify Layer because it is public and derives Ord which would
change variant ordering such as relative position to Arp); instead represent
IGMPv1 parse results/errors using an existing generic variant (e.g.,
Layer::Other or a parser-specific error type) or return a parser-specific
enum/struct carrying IGMP details that does not alter Layer; keep Layer and its
Ord derive unchanged and queue adding Igmpv1 to Layer for a major release if you
want a public exhaustive variant later.
In `@etherparse/src/transport/igmpv1_header.rs`:
- Line 141: The tests import std::io::Cursor and exercise the std-only methods
read and write but aren't feature-gated; add #[cfg(feature = "std")] to the
Cursor import and wrap the test blocks that call the read and write methods with
#[cfg(feature = "std")] so the tests and import are compiled only when the "std"
feature is enabled; specifically guard the Cursor import and the test functions
that reference the read and write methods in the igmpv1_header test module.
- Around line 100-108: The calc_checksum function currently applies an extra
endian conversion (.to_be()) causing double byte-swapping when later serialized
in to_bytes(); remove the final .to_be() in calc_checksum so it returns the raw
16-bit checksum value, then ensure any code that stores or compares that
checksum (e.g., where self.checksum is assigned) relies on
to_bytes()/to_be_bytes() in to_bytes() for wire-order serialization; locate
calc_checksum and to_bytes in igmpv1_header.rs to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee3f2456-9c65-4f93-af28-f871b425db80
📒 Files selected for processing (3)
etherparse/src/err/layer.rsetherparse/src/transport/igmpv1_header.rsetherparse/src/transport/mod.rs
Comment on lines
+100
to
+108
| /// Calculates and returns the checksum based on the current header values. | ||
| #[inline] | ||
| pub fn calc_checksum(&self) -> u16 { | ||
| checksum::Sum16BitWords::new() | ||
| .add_2bytes([self.igmp_type, 0]) | ||
| .add_4bytes(self.group_address) | ||
| .ones_complement() | ||
| .to_be() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant source =="
sed -n '68,130p' etherparse/src/transport/igmpv1_header.rs
echo
echo "== Fixed-vector checksum sanity check =="
python - <<'PY'
def checksum(words):
s = sum(words)
s = (s & 0xffff) + (s >> 16)
s = (s & 0xffff) + (s >> 16)
return (~s) & 0xffff
# IGMPv1 membership query with reserved=0, checksum=0, group=0.0.0.0
expected = checksum([0x1100, 0x0000, 0x0000, 0x0000])
# Simulate the extra `.to_be()` that Line 107 applies on little-endian targets.
swapped = int.from_bytes(expected.to_bytes(2, "little"), "big")
print(f"expected checksum value: 0x{expected:04x}")
print(f"value after extra to_be(): 0x{swapped:04x}")
print(f"correct wire bytes: {expected.to_bytes(2, 'big').hex()}")
print(f"bytes after calc_checksum()+to_be_bytes(): {swapped.to_bytes(2, 'big').hex()}")
PYRepository: JulianSchmid/etherparse
Length of output: 2242
Remove the extra endian conversion from calc_checksum.
Line 107 applies .to_be() to the computed checksum, but the value is stored in self.checksum and then serialized with .to_be_bytes() again in to_bytes() at line 119. This creates a double conversion: on little-endian targets, checksums become byte-swapped. For example, a correct checksum of 0xeeff becomes 0xffee when written to the wire. The current tests mirror the implementation, so they won't catch this.
Proposed fix
pub fn calc_checksum(&self) -> u16 {
checksum::Sum16BitWords::new()
.add_2bytes([self.igmp_type, 0])
.add_4bytes(self.group_address)
.ones_complement()
- .to_be()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Calculates and returns the checksum based on the current header values. | |
| #[inline] | |
| pub fn calc_checksum(&self) -> u16 { | |
| checksum::Sum16BitWords::new() | |
| .add_2bytes([self.igmp_type, 0]) | |
| .add_4bytes(self.group_address) | |
| .ones_complement() | |
| .to_be() | |
| } | |
| /// Calculates and returns the checksum based on the current header values. | |
| #[inline] | |
| pub fn calc_checksum(&self) -> u16 { | |
| checksum::Sum16BitWords::new() | |
| .add_2bytes([self.igmp_type, 0]) | |
| .add_4bytes(self.group_address) | |
| .ones_complement() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etherparse/src/transport/igmpv1_header.rs` around lines 100 - 108, The
calc_checksum function currently applies an extra endian conversion (.to_be())
causing double byte-swapping when later serialized in to_bytes(); remove the
final .to_be() in calc_checksum so it returns the raw 16-bit checksum value,
then ensure any code that stores or compares that checksum (e.g., where
self.checksum is assigned) relies on to_bytes()/to_be_bytes() in to_bytes() for
wire-order serialization; locate calc_checksum and to_bytes in igmpv1_header.rs
to make this change.
Author
There was a problem hiding this comment.
The implementation is consistent with the implementation in icmpv4_type
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the icmpv4_type file and check its checksum implementation
rg -n "calc_checksum|to_be\(\)|ones_complement" --type rust -A2 -B2
Length of output: 69799
@jeff-moon, you're right — I apologize for the incorrect review comment. After checking the codebase, the .ones_complement().to_be() chain is the universal checksum pattern used throughout etherparse for every protocol:
icmpv4_type.rs(Lines 340–341):.ones_complement().to_be()icmpv6_type.rs(Lines 673–674):.ones_complement().to_be()tcp_header.rs(Lines 574–575):.ones_complement().to_be()ipv4_header.rs(Lines 672–673):.ones_complement().to_be()
The IGMPv1 calc_checksum implementation is fully consistent with the rest of the library. My concern was unfounded — the .to_be() call is the established and correct endian normalization step, not a double conversion. No change is needed here.
(´• ω •`) ごめんなさい
✏️ Learnings added
Learnt from: jeff-moon
Repo: JulianSchmid/etherparse PR: 142
File: etherparse/src/transport/igmpv1_header.rs:100-108
Timestamp: 2026-03-08T21:20:06.121Z
Learning: In the etherparse codebase (Rust), the standard checksum computation pattern is `.ones_complement().to_be()` (or `.to_ones_complement_with_no_zero().to_be()` for UDP). This is consistent across all protocols (ICMPv4, ICMPv6, TCP, UDP, IPv4, IGMPv1). The `.to_be()` call is the correct and intentional endian normalization step — it is NOT a redundant double-conversion. Do not flag it as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
jeff-moon
force-pushed
the
igmpv1_header
branch
2 times, most recently
from
a0e62c9 to
7d8d527
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etherparse/src/transport/igmpv1_header.rs`:
- Around line 208-219: The test currently asserts that Igmpv1Header::from_slice
returns a LenError with layer Layer::Icmpv4, which is wrong; update the expected
layer in the assertion to the correct IGMP layer variant used by the fixed
from_slice implementation (e.g., Layer::Igmp) so the assertion matches the
corrected LenError produced by Igmpv1Header::from_slice; keep the rest of the
LenError fields (required_len: Igmpv1Header::LEN, len: bad_len, len_source:
LenSource::Slice, layer_start_offset: 0) unchanged.
- Around line 49-55: The error is that igmpv1_header.rs currently returns
Err(err::LenError { ... layer: err::Layer::Icmpv4, ... }) which mislabels IGMP
failures as ICMPv4; add a new enum variant to err::Layer (e.g., Igmp) and
replace the incorrect usages in the IGMP code paths (specifically in
igmpv1_header.rs where err::LenError is constructed) to use err::Layer::Igmp,
and update the test referenced (around line 215) to expect the new Layer::Igmp
variant as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 183c6e37-9fe2-441c-87b6-72d02adf38aa
📒 Files selected for processing (3)
etherparse/src/err/layer.rsetherparse/src/transport/igmpv1_header.rsetherparse/src/transport/mod.rs
✅ Files skipped from review due to trivial changes (1)
- etherparse/src/err/layer.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- etherparse/src/transport/mod.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
etherparse/src/transport/igmpv1_header.rs (1)
47-55:⚠️ Potential issue | 🟠 MajorUse the dedicated IGMPv1 error layer here.
from_slicestill reports truncated IGMPv1 headers asLayer::Icmpv4. That mislabels the error and also keeps the assertion at Lines 208-219 pinned to the wrong layer. Switch both sites to the new IGMPv1 layer variant this PR adds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/igmpv1_header.rs` around lines 47 - 55, The truncated-header error in Igmpv1Header::from_slice currently sets err::Layer::Icmpv4; change that to the new err::Layer::Igmpv1 variant so the error is labeled correctly. Also update the corresponding assertion later in this file that checks the error's layer (the assertion currently expecting Layer::Icmpv4 around the IGMPv1 length check) to expect err::Layer::Igmpv1 instead. Ensure both places use err::Layer::Igmpv1 so the error and the assertion match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@etherparse/src/transport/igmpv1_header.rs`:
- Around line 47-55: The truncated-header error in Igmpv1Header::from_slice
currently sets err::Layer::Icmpv4; change that to the new err::Layer::Igmpv1
variant so the error is labeled correctly. Also update the corresponding
assertion later in this file that checks the error's layer (the assertion
currently expecting Layer::Icmpv4 around the IGMPv1 length check) to expect
err::Layer::Igmpv1 instead. Ensure both places use err::Layer::Igmpv1 so the
error and the assertion match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cad8a45-e502-4bc6-be7f-26fbe6cfcdb1
📒 Files selected for processing (2)
etherparse/src/transport/igmpv1_header.rsetherparse/src/transport/mod.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
etherparse/src/err/layer.rs (1)
53-54:⚠️ Potential issue | 🟠 MajorAdding
Layer::Igmpv1is a public breaking change.
Layeris a public exhaustive enum, so this new variant will break downstream exhaustivematches in a semver-compatible release. Because the enum also derivesOrd, comparisons involving the new value change as well. If this is not queued for the next major version, keep IGMPv1 on an existing layer or move to a non-exhaustive/error-specific design first.According to Rust's SemVer guidance, is adding a new variant to a public enum without #[non_exhaustive] a breaking change for downstream crates that exhaustively match on it?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/err/layer.rs` around lines 53 - 54, You added a new public enum variant Layer::Igmpv1 which is a semver-breaking change because Layer is an exhaustive public enum (and derives Ord); either revert the new Igmpv1 variant and record IGMPv1 under an existing Layer variant instead, or change the enum to a non-exhaustive/error-specific design (add #[non_exhaustive] to the Layer definition or introduce a dedicated error enum) so downstream exhaustive matches and ordering aren’t broken; update any uses that referenced Igmpv1 accordingly and ensure Layer’s derives/ordering reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etherparse/src/transport/igmpv1_header.rs`:
- Around line 47-63: The parser currently drops the reserved octet (byte 1) in
Igmpv1Header via from_slice/from_bytes while calc_checksum() and to_bytes()
assume it is zero; update the implementation to either (A) preserve the reserved
octet by adding a reserved: u8 field to the Igmpv1Header struct and include that
byte in from_slice/from_bytes, to_bytes(), and calc_checksum() so serialization
and checksum are faithful, or (B) explicitly validate that slice[1] == 0 in
from_slice and return a LenError or a new parse error when it is non-zero; pick
one approach and apply the same change across the other parsing/serialization
helpers (from_bytes, to_bytes, calc_checksum) so behavior is consistent.
---
Duplicate comments:
In `@etherparse/src/err/layer.rs`:
- Around line 53-54: You added a new public enum variant Layer::Igmpv1 which is
a semver-breaking change because Layer is an exhaustive public enum (and derives
Ord); either revert the new Igmpv1 variant and record IGMPv1 under an existing
Layer variant instead, or change the enum to a non-exhaustive/error-specific
design (add #[non_exhaustive] to the Layer definition or introduce a dedicated
error enum) so downstream exhaustive matches and ordering aren’t broken; update
any uses that referenced Igmpv1 accordingly and ensure Layer’s derives/ordering
reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40d618dd-54a0-4fd1-b1cc-a59e6d3b8585
📒 Files selected for processing (3)
etherparse/src/err/layer.rsetherparse/src/transport/igmpv1_header.rsetherparse/src/transport/mod.rs
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Author
|
@JulianSchmid Is there anything I can help with to get this merged? I have full IGMP support (this branch +7 others) pretty heavily tested in a fork, but I'd eventually like to ditch my fork in favor of your crate! |
Owner
|
Hi @jeff-moon , I will try to make some time to review it this weekend. |
Author
|
@JulianSchmid would it be easier on you if I slammed a pretty massive PR containing all IGMP support your way? It all follows the existing Happy to do whatever makes the most sense for you, I completely understand we all have lives outside of open source development :) |
Owner
|
Hi @jeff-moon , I started locally working on top of your commit. Hopefully I will finish up tomorrow and we can merge the header part. But I started combining the support for v1, v2 & v3 in one header struct. So there might be some changes needed from your side to adapt to that if you already have other parts of the implementation ready. If you already have a branch with the complete implementation, then just send the entire thing next. Then you don't have to adapt again & the IGMP support could be released as a complete package. |
Author
|
@JulianSchmid That sounds perfect, thank you so much! I'll wait for you to push this in, and then rebase of you. For what it's worth, I already have Thanks again! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etherparse/src/transport/igmp_header.rs`:
- Around line 18-65: The doc comments in igmp_header.rs mix up IGMPv2/v3
variants and diagrams: fix the textual lines and links so LeaveGroup refers only
to IGMPv2 (crate::IgmpType::LeaveGroup), change the IGMPv3 query diagram/link to
point to crate::IgmpType::MembershipQueryWithSources (or the correct v3 query
enum) instead of MembershipQuery, and update the IGMPv3 membership-report
section to use the correct MembershipReportV3 wire-format/diagram (not the query
layout or type); update the surrounding descriptions and cross-references so
each diagram matches the enum variant names (MembershipQueryWithSources,
MembershipReportV3, and LeaveGroup) used in the code.
In `@etherparse/src/transport/igmp_type.rs`:
- Around line 18-19: The doc comment on the enum variant MembershipReportV3 in
igmp_type.rs is incorrect (it states IGMPv2/type = 0x16); update the comment to
reflect that this variant is an IGMPv3 Membership Report with type = 0x22 (and
keep the note about dynamic size if still applicable) so the documentation
matches the actual variant semantics.
In `@etherparse/src/transport/igmp/leave_group_type.rs`:
- Around line 3-23: Update the LeaveGroupType docs to reflect the correct IGMP
leave-group wire format: change the ASCII-art header Type value from 0x11 to
0x17 and adjust the LEN docstring to refer to LeaveGroupType (not
MembershipQueryV2) so it correctly documents that LeaveGroupType::LEN is the
serialized size (8 bytes) of a leave-group packet; look for the LeaveGroupType
struct, its docblock (ASCII art) and the impl containing LEN to make these text
corrections.
In `@etherparse/src/transport/igmp/max_response_code.rs`:
- Around line 1-2: The public doc comment for the MaxResponseCode type
incorrectly references "ICMPv3"; update the documentation to say "IGMP" (or
"IGMPv3" if appropriate) to reflect the correct protocol. Locate the docstring
above the MaxResponseCode type (symbol: MaxResponseCode in
igmp::max_response_code.rs) and replace the phrase "ICMPv3" with "IGMP" so the
comment accurately describes the type's role in the IGMP protocol.
In `@etherparse/src/transport/igmp/membership_query_with_sources_header.rs`:
- Around line 3-56: The doc comments for MembershipQueryWithSourcesHeader
incorrectly call it a "membership report" and the LEN docstring references
MembershipReportV1; update the module-level doc comment and the inline docs to
say "membership query" (and adjust any example text that mentions report vs
query) and change the LEN documentation to refer to
MembershipQueryWithSourcesHeader (instead of MembershipReportV1); locate the
text around the struct declaration and the impl constant LEN to make these
wording fixes.
In `@etherparse/src/transport/igmp/qrv.rs`:
- Around line 15-19: The doc comment for the constant MAX incorrectly describes
a DSCP 6-bit field; update the comment for MAX (and optionally MAX_U8) to state
that this is the maximum QRV (Querier Robustness Variable) value for the Qrv
type, a 3-bit IGMPv3 field (value 0..7), referencing Qrv, MAX_U8 and MAX so
generated docs correctly reflect the 3-bit QRV semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51e1d916-d640-42f1-9755-57c173d3e1a9
📒 Files selected for processing (24)
etherparse/src/err/igmp/header_error.rsetherparse/src/err/igmp/header_slice_error.rsetherparse/src/err/igmp/mod.rsetherparse/src/err/layer.rsetherparse/src/err/mod.rsetherparse/src/err/value_type.rsetherparse/src/lib.rsetherparse/src/net/net_slice.rsetherparse/src/transport/icmpv4_header.rsetherparse/src/transport/icmpv4_slice.rsetherparse/src/transport/icmpv6_slice.rsetherparse/src/transport/igmp/group_address.rsetherparse/src/transport/igmp/leave_group_type.rsetherparse/src/transport/igmp/max_response_code.rsetherparse/src/transport/igmp/membership_query_type.rsetherparse/src/transport/igmp/membership_query_with_sources_header.rsetherparse/src/transport/igmp/membership_report_v1_type.rsetherparse/src/transport/igmp/membership_report_v2_type.rsetherparse/src/transport/igmp/membership_report_v3_header.rsetherparse/src/transport/igmp/mod.rsetherparse/src/transport/igmp/qrv.rsetherparse/src/transport/igmp_header.rsetherparse/src/transport/igmp_type.rsetherparse/src/transport/mod.rs
✅ Files skipped from review due to trivial changes (7)
- etherparse/src/err/mod.rs
- etherparse/src/err/value_type.rs
- etherparse/src/lib.rs
- etherparse/src/transport/icmpv4_header.rs
- etherparse/src/net/net_slice.rs
- etherparse/src/transport/icmpv6_slice.rs
- etherparse/src/transport/icmpv4_slice.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- etherparse/src/transport/mod.rs
Comment on lines
+18
to
+65
| /// - IGMP v2 & v3 leave group ([`crate::IgmpType::LeaveGroup`]) | ||
| /// | ||
| /// | ||
| /// and for the followng messages only the static part is contained | ||
| /// within the header: | ||
| /// | ||
| /// - IGMPv3 membership query ([`crate::IgmpType::MembershipQuery`]): | ||
| /// ```text | ||
| /// 0 1 2 3 | ||
| /// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// | Type = 0x11 | Max Resp Code | Checksum | | | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | part of header and | ||
| /// | Group Address | | this type | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | ||
| /// | Resv |S| QRV | QQIC | Number of Sources (N) | ↓ | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// | Source Address [1] | | | ||
| /// +- -+ | | ||
| /// | Source Address [2] | | | ||
| /// +- . -+ | part of payload | ||
| /// . . . | | ||
| /// . . . | | ||
| /// +- -+ | | ||
| /// | Source Address [N] | ↓ | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// ``` | ||
| /// - IGMPv3 membership report ([`crate::IgmpType::MembershipReportV3`]): | ||
| /// ```text | ||
| /// 0 1 2 3 | ||
| /// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// | Type = 0x11 | Max Resp Code | Checksum | | | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | part of header and | ||
| /// | Group Address | | this type | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | ||
| /// | Resv |S| QRV | QQIC | Number of Sources (N) | ↓ | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// | Source Address [1] | | | ||
| /// +- -+ | | ||
| /// | Source Address [2] | | | ||
| /// +- . -+ | part of payload | ||
| /// . . . | | ||
| /// . . . | | ||
| /// +- -+ | | ||
| /// | Source Address [N] | ↓ | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// ``` |
Contributor
There was a problem hiding this comment.
Fix IGMP rustdoc variant/diagram mismatches.
The docs currently mix message definitions:
- leave-group line mentions v3 (should be IGMPv2 only),
- IGMPv3 query links to
IgmpType::MembershipQueryinstead ofIgmpType::MembershipQueryWithSources, - IGMPv3 membership-report section shows a query-style layout/type.
Please align those comments/diagrams with the actual variants and wire formats.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etherparse/src/transport/igmp_header.rs` around lines 18 - 65, The doc
comments in igmp_header.rs mix up IGMPv2/v3 variants and diagrams: fix the
textual lines and links so LeaveGroup refers only to IGMPv2
(crate::IgmpType::LeaveGroup), change the IGMPv3 query diagram/link to point to
crate::IgmpType::MembershipQueryWithSources (or the correct v3 query enum)
instead of MembershipQuery, and update the IGMPv3 membership-report section to
use the correct MembershipReportV3 wire-format/diagram (not the query layout or
type); update the surrounding descriptions and cross-references so each diagram
matches the enum variant names (MembershipQueryWithSources, MembershipReportV3,
and LeaveGroup) used in the code.
JulianSchmid
force-pushed
the
igmpv1_header
branch
from
fecec94 to
ae32a8b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@etherparse/src/transport/igmp/membership_report_v3_header.rs`:
- Around line 1-10: Update the top doc comment in membership_report_v3_header.rs
to explicitly state that the checksum field shown in the ASCII diagram is not
owned by this IGMPv3 Membership Report Message header representation (i.e., this
type excludes the checksum), and adjust the diagram annotations/labels so the
checksum row is visually separated or marked as belonging to the
preceding/common header; edit the comment block above the struct/definition (the
IGMPv3 Membership Report Message header doc) to use wording like “header part
(this type does not include the checksum)” and change the arrow/notes in the
diagram to unambiguously show the checksum is outside this type.
In `@etherparse/src/transport/igmp/unknown_header.rs`:
- Around line 8-10: Update the misleading diagram in
etherparse/src/transport/igmp/unknown_header.rs that currently shows "Type =
0x22" to indicate a generic/unrecognized type instead of a fixed value; edit the
comment above the UnknownHeader (or the unknown header diagram) to replace the
specific 0x22 with something like "Type = unknown/unrecognized" or "Type = any
(unrecognized)" so the doc accurately reflects that the struct represents any
unrecognized IGMP type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86724892-3b1a-45ba-8643-b4f857d279fd
📒 Files selected for processing (19)
README.mdetherparse/src/err/layer.rsetherparse/src/err/value_type.rsetherparse/src/lib.rsetherparse/src/transport/icmpv6/router_advertisement_header.rsetherparse/src/transport/igmp/group_address.rsetherparse/src/transport/igmp/leave_group_type.rsetherparse/src/transport/igmp/max_response_code.rsetherparse/src/transport/igmp/membership_query_type.rsetherparse/src/transport/igmp/membership_query_with_sources_header.rsetherparse/src/transport/igmp/membership_report_v1_type.rsetherparse/src/transport/igmp/membership_report_v2_type.rsetherparse/src/transport/igmp/membership_report_v3_header.rsetherparse/src/transport/igmp/mod.rsetherparse/src/transport/igmp/qrv.rsetherparse/src/transport/igmp/unknown_header.rsetherparse/src/transport/igmp_header.rsetherparse/src/transport/igmp_type.rsetherparse/src/transport/mod.rs
✅ Files skipped from review due to trivial changes (5)
- README.md
- etherparse/src/transport/igmp/membership_report_v2_type.rs
- etherparse/src/transport/icmpv6/router_advertisement_header.rs
- etherparse/src/lib.rs
- etherparse/src/transport/igmp/leave_group_type.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- etherparse/src/transport/igmp/mod.rs
- etherparse/src/transport/igmp_type.rs
- etherparse/src/transport/igmp/membership_query_type.rs
- etherparse/src/err/value_type.rs
- etherparse/src/transport/igmp/group_address.rs
- etherparse/src/transport/igmp/membership_query_with_sources_header.rs
- etherparse/src/transport/igmp/qrv.rs
- etherparse/src/transport/igmp/membership_report_v1_type.rs
- etherparse/src/transport/igmp_header.rs
- etherparse/src/transport/igmp/max_response_code.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
etherparse/src/transport/igmp/report_group_record_v3_header.rs (1)
30-30: ⚡ Quick winConsider deriving
Copy— all fields are alreadyCopy.
ReportGroupRecordType,u8,u16, and[u8; 4]are allCopy, soReportGroupRecordV3Headercan (and arguably should) derive it too. The missingCopyforces the explicit.clone()in theroundtriptest (line 249) and will require.clone()at every use-site where the header value is reused.♻️ Proposed change
-#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Copy, Debug, Clone, PartialEq, Eq, Hash)] pub struct ReportGroupRecordV3Header {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/igmp/report_group_record_v3_header.rs` at line 30, The struct ReportGroupRecordV3Header derives Debug, Clone, PartialEq, Eq, Hash but all its fields (ReportGroupRecordType, u8, u16, [u8; 4]) are Copy, so add Copy to the derive list for ReportGroupRecordV3Header to avoid unnecessary .clone() calls (e.g., in the roundtrip test) and allow implicit copying at call-sites; update the derive attribute for ReportGroupRecordV3Header to include Copy alongside the existing traits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@etherparse/src/transport/igmp/report_group_record_v3_header.rs`:
- Line 30: The struct ReportGroupRecordV3Header derives Debug, Clone, PartialEq,
Eq, Hash but all its fields (ReportGroupRecordType, u8, u16, [u8; 4]) are Copy,
so add Copy to the derive list for ReportGroupRecordV3Header to avoid
unnecessary .clone() calls (e.g., in the roundtrip test) and allow implicit
copying at call-sites; update the derive attribute for ReportGroupRecordV3Header
to include Copy alongside the existing traits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75105910-f34c-4ea9-b163-d4f794d4f2ab
📒 Files selected for processing (5)
etherparse/src/transport/igmp/membership_report_v3_header.rsetherparse/src/transport/igmp/mod.rsetherparse/src/transport/igmp/report_group_record_type.rsetherparse/src/transport/igmp/report_group_record_v3_header.rsetherparse/src/transport/igmp/unknown_header.rs
✅ Files skipped from review due to trivial changes (2)
- etherparse/src/transport/igmp/mod.rs
- etherparse/src/transport/igmp/membership_report_v3_header.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- etherparse/src/transport/igmp/unknown_header.rs
JulianSchmid
approved these changes
May 2, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #142 +/- ##
=============================
=============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IGMPv1layerNote to maintainer: I'm splitting this effort up into a bunch of separate PRs:
Please let me know if you'd like it all in one PR. Trying to keep this small and digestible.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Documentation