IGMPv1 header support#142
Open
jeff-moon wants to merge 6 commits intoJulianSchmid:masterfrom
Open
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThe pull request adds comprehensive IGMP (Internet Group Management Protocol) support to etherparse, introducing new data structures for IGMP message types (membership queries, reports, leave groups), error handling infrastructure, header parsing with version detection, checksum computation, and serialization logic. Changes
Sequence DiagramsequenceDiagram
actor User
participant IgmpHeader
participant TypeDispatch
participant QueryDetector
participant Parser
User->>IgmpHeader: from_slice(slice)
IgmpHeader->>Parser: validate minimum 8 bytes
alt Slice too short
Parser-->>IgmpHeader: HeaderSliceError::Len
IgmpHeader-->>User: Error
else Valid length
Parser->>Parser: extract common fields<br/>(type, code, checksum)
IgmpHeader->>TypeDispatch: dispatch by type byte
alt Type 0x11 (Membership Query)
TypeDispatch->>QueryDetector: check slice length
alt Length == 8 bytes
QueryDetector->>QueryDetector: parse as v1/v2
QueryDetector-->>TypeDispatch: MembershipQuery
else Length >= 12 bytes
QueryDetector->>QueryDetector: parse as v3
QueryDetector-->>TypeDispatch: MembershipQueryWithSources
end
else Other types
TypeDispatch->>TypeDispatch: direct type parsing
end
TypeDispatch-->>IgmpHeader: IgmpType variant
IgmpHeader-->>User: (IgmpHeader, remaining_slice)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 37 minutes and 14 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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() | ||
| } |
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
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
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
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
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 |
✅ 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! |
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] | ↓ | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// ``` |
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.
Comment on lines
+3
to
+23
| /// A leave group message type (introduced in IGMPv2). | ||
| /// | ||
| /// ```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 | 0 | Checksum | | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| /// | Group Address | | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| /// ``` | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct LeaveGroupType { | ||
| /// The IP multicast group address of the group being left. | ||
| pub group_address: GroupAddress, | ||
| } | ||
|
|
||
| impl LeaveGroupType { | ||
| /// Number of bytes/octets an [`MembershipQueryV2`] takes up in serialized form. | ||
| pub const LEN: usize = 8; | ||
| } |
There was a problem hiding this comment.
Correct the IGMP leave-group docs.
The ASCII art still shows type 0x11, and the LEN doc references MembershipQueryV2; leave-group packets are 0x17, so this block will teach the wrong wire format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etherparse/src/transport/igmp/leave_group_type.rs` around lines 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.
Comment on lines
+3
to
+56
| /// A membership report message type (IGMPv3 version) with source addresses. | ||
| /// | ||
| /// ```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] | ↓ | ||
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | ||
| /// ``` | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct MembershipQueryWithSourcesHeader { | ||
| /// The Max Resp Code field specifies the maximum time allowed before | ||
| /// sending a responding report. | ||
| pub max_response_code: MaxResponseCode, | ||
|
|
||
| /// The group address being queried. | ||
| /// | ||
| /// Set to zero for general queries, to learn which groups | ||
| /// have members on an attached network. Filled for group-specific | ||
| /// queries to learn if a particular group has members on an | ||
| /// attached network. | ||
| /// | ||
| /// For IGMPv1, this field is always set to zero. | ||
| pub group_address: GroupAddress, | ||
|
|
||
| /// S Flag (Suppress Router-Side Processing). | ||
| pub s_flag: bool, | ||
|
|
||
| /// QRV (Querier's Robustness Variable) | ||
| pub qrv: Qrv, | ||
|
|
||
| /// QQIC (Querier's Query Interval Code) | ||
| pub qqic: u8, | ||
|
|
||
| /// Number of sources | ||
| pub num_of_sources: u16, | ||
| } | ||
|
|
||
| impl MembershipQueryWithSourcesHeader { | ||
| /// Number of bytes/octets an [`MembershipReportV1`] takes up in serialized form. | ||
| pub const LEN: usize = 12; |
There was a problem hiding this comment.
Correct the public doc wording for this header type.
The type is a membership query header, but docs currently call it a membership report and reference the wrong struct in LEN docs.
Suggested patch
-/// A membership report message type (IGMPv3 version) with source addresses.
+/// A membership query message type (IGMPv3 version) with source addresses.
@@
- /// Number of bytes/octets an [`MembershipReportV1`] takes up in serialized form.
+ /// Number of bytes/octets a [`MembershipQueryWithSourcesHeader`] takes up in serialized form.📝 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
| /// A membership report message type (IGMPv3 version) with source addresses. | |
| /// | |
| /// ```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] | ↓ | |
| /// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ - | |
| /// ``` | |
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | |
| pub struct MembershipQueryWithSourcesHeader { | |
| /// The Max Resp Code field specifies the maximum time allowed before | |
| /// sending a responding report. | |
| pub max_response_code: MaxResponseCode, | |
| /// The group address being queried. | |
| /// | |
| /// Set to zero for general queries, to learn which groups | |
| /// have members on an attached network. Filled for group-specific | |
| /// queries to learn if a particular group has members on an | |
| /// attached network. | |
| /// | |
| /// For IGMPv1, this field is always set to zero. | |
| pub group_address: GroupAddress, | |
| /// S Flag (Suppress Router-Side Processing). | |
| pub s_flag: bool, | |
| /// QRV (Querier's Robustness Variable) | |
| pub qrv: Qrv, | |
| /// QQIC (Querier's Query Interval Code) | |
| pub qqic: u8, | |
| /// Number of sources | |
| pub num_of_sources: u16, | |
| } | |
| impl MembershipQueryWithSourcesHeader { | |
| /// Number of bytes/octets an [`MembershipReportV1`] takes up in serialized form. | |
| pub const LEN: usize = 12; | |
| /// A membership query message type (IGMPv3 version) with source addresses. | |
| /// | |
| /// |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etherparse/src/transport/igmp/membership_query_with_sources_header.rs` around
lines 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.
JulianSchmid
force-pushed
the
igmpv1_header
branch
from
fecec94 to
ae32a8b
Compare
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
Release Notes
New Features
Refactor