Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new IGMPv1 transport header module with parsing, construction, serialization, checksum utilities, public constants and types; registers the module in transport exports; and adds an Igmpv1 variant to the error Layer enum and related tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
| /// Error occurred while parsing an IGMPv1 packet. | ||
| Igmpv1, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to Rust semver guidance, is adding a new variant to a public enum without #[non_exhaustive] a breaking change for downstream exhaustive matches and derived ordering?
💡 Result:
Yes. Rust’s SemVer guidance (Cargo Book) explicitly classifies “adding new enum variants (without #[non_exhaustive])” as a major/breaking change, because it will break downstream crates that do exhaustive match on the enum (they become non-exhaustive and fail to compile). [1]
It can also break derived ordering expectations: Rust’s derived Ord/PartialOrd for enums orders variants by their discriminant, which by default follows declaration order (top variants compare smaller than later variants). Adding a new variant therefore changes the set of values and their relative ordering involving the new variant (and can affect downstream code that relies on sorting/ordering over the enum’s full domain). [2]
Sources:
[1] Cargo Book: SemVer Compatibility — “Major: adding new enum variants (without non_exhaustive)”
[2] Rust standard library docs: std::cmp::Ord (derived ordering for enums uses discriminant / variant order)
Queue this enum extension for a major release.
Layer is a public exhaustive enum, so adding Igmpv1 will break downstream exhaustive match statements. Because the enum also derives Ord, inserting Igmpv1 here changes the relative ordering of Arp and other existing variants. If this PR is intended for a semver-compatible release, avoid extending Layer directly.
🤖 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, 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.
There was a problem hiding this comment.
There's no other meaningful error to use, so I think we need to add this. My plan is a layer for each version (like Icmp), but can update for a single Igmp error if maintainers think this is best
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the crate version in Cargo.toml
grep -m1 '^\s*version\s*=' etherparse/Cargo.toml || grep -m1 '^\s*version\s*=' Cargo.toml
Length of output: 18
@jeff-moon, thanks for the context. The crate is at 0.19.0, so under Cargo's SemVer conventions for 0.x crates, adding enum variants is a breaking change but is permitted within a minor-version bump (e.g. 0.19 → 0.20). That significantly reduces the concern I raised.
The per-version naming (e.g. Igmpv1, Igmpv2 to mirror Icmpv4/Icmpv6) is a reasonable and consistent pattern for this codebase.
One forward-looking suggestion worth considering: annotating Layer with #[non_exhaustive] would be a one-time change that makes all future protocol additions (IGMP versions, etc.) non-breaking once the crate reaches 1.0. That said, this is a broader design decision for the maintainer and out of scope for this specific PR.
I'll leave the Igmpv1 vs. single Igmp variant choice to the maintainer as you suggested. The current approach looks fine given the 0.x versioning.
🧠 Learnings used
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 Rust codebase, the standard checksum computation pattern across protocols is to apply ones_complement() followed by to_be() (or to_ones_complement_with_no_zero() then to_be() for UDP). This to_be() step is the intentional endianness normalization, not a redundant double-conversion. Do not flag this as a bug. Applies broadly to checksum handling for ICMPv4, ICMPv6, TCP, UDP, IPv4, and IGMPv1 within this crate; ensure checksums consistently use the ones-complement representation with big-endian ordering when constructing or validating headers.
| /// 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.
| /// 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.
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
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
1 participant
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