Add IgmpSlice for zero-copy IGMP parsing#148
Add IgmpSlice for zero-copy IGMP parsing#148jeff-moon wants to merge 1 commit intoJulianSchmid:masterfrom
Conversation
Adds zero-copy slice types for IGMP packets covering v1, v2, and v3: - IgmpSlice: wraps raw IGMP packet bytes with field accessors - ReportGroupRecordV3Slice: zero-copy access to IGMPv3 group records - ReportGroupRecordV3SliceIter: iterates group records in v3 reports
📝 WalkthroughWalkthroughThis PR introduces zero-copy slice-based parsing for IGMPv3 group records and general IGMP packets. Two new slice types ( ChangesIGMP v3 Slice Parsing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
|
Thank you for the work you did on the previous IGMP PR @JulianSchmid! I've updated my IGMP slice parsing to conform to the changes you've made. Please let me know if there's any changes you would like me to make here! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
etherparse/src/transport/igmp_slice.rs (1)
230-258: 💤 Low valueMinor: redundant slicing in test.
bytesis already[u8; 16], so&bytes[..16]is equivalent to&bytes. Not worth fixing on its own — flagging only in case you touch this test for other reasons.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etherparse/src/transport/igmp_slice.rs` around lines 230 - 258, In the from_slice_v3_query test, remove the redundant slice expression when calling IgmpSlice::from_slice by passing the byte array directly (replace &bytes[..16] with &bytes) to simplify the test; update the call site in the from_slice_v3_query test that invokes IgmpSlice::from_slice and keep all other assertions and uses of bytes unchanged.etherparse/src/transport/igmp/report_group_record_v3_slice.rs (1)
148-152: 💤 Low valueDoc nit: clarify what
slice()returns vs. the constructor input.Worth noting in the doc that
slice()is exactlyrecord_lenbytes (the full record), not the original slice passed tofrom_slice. This contrasts withIgmpSlice::slice(), which exposes the full input including trailing payload — small clarification will help users avoid confusion.📝 Suggested doc tweak
- /// Returns the full slice of this group record. + /// Returns the bytes of this group record (header + sources + aux data). + /// + /// Note: this is exactly the validated record length, not any + /// trailing bytes from the original input passed to + /// [`ReportGroupRecordV3Slice::from_slice`] (those are returned + /// separately as the second element of the tuple). #[inline] pub fn slice(&self) -> &'a [u8] { self.slice }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etherparse/src/transport/igmp/report_group_record_v3_slice.rs` around lines 148 - 152, Update the doc comment for the method slice() on ReportGroupRecordV3Slice to explicitly state that it returns exactly the record_len bytes of the group record (the full record slice stored in the struct) rather than the original input slice passed to from_slice; mention the contrast with IgmpSlice::slice() which exposes the entire input including trailing payload so callers understand slice() here is trimmed to the record length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@etherparse/src/transport/igmp_slice.rs`:
- Around line 230-258: In the from_slice_v3_query test, remove the redundant
slice expression when calling IgmpSlice::from_slice by passing the byte array
directly (replace &bytes[..16] with &bytes) to simplify the test; update the
call site in the from_slice_v3_query test that invokes IgmpSlice::from_slice and
keep all other assertions and uses of bytes unchanged.
In `@etherparse/src/transport/igmp/report_group_record_v3_slice.rs`:
- Around line 148-152: Update the doc comment for the method slice() on
ReportGroupRecordV3Slice to explicitly state that it returns exactly the
record_len bytes of the group record (the full record slice stored in the
struct) rather than the original input slice passed to from_slice; mention the
contrast with IgmpSlice::slice() which exposes the entire input including
trailing payload so callers understand slice() here is trimmed to the record
length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b14abde8-90f2-450f-a770-9f0f791ace9e
📒 Files selected for processing (4)
etherparse/src/transport/igmp/mod.rsetherparse/src/transport/igmp/report_group_record_v3_slice.rsetherparse/src/transport/igmp_slice.rsetherparse/src/transport/mod.rs
Adds zero-copy slice types for IGMP packets covering v1, v2, and v3:
Summary by CodeRabbit