Add structured ICMPv6 ND payload and option slice support#140
Merged
JulianSchmid merged 1 commit intoJulianSchmid:masterfrom Mar 8, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds comprehensive ICMPv6 support: owned payload types, borrowed payload-slice views, a typed NDP option parsing framework (headers, slice types, iterator, and read errors), integration into ICMPv6 decoding, and a single-line formatting tweak in an ICMPv4 test. No existing public signatures removed. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Icmpv6Slice
participant Icmpv6PayloadSlice
participant PayloadSliceType
participant NdpOptionsIterator
participant OwnedPayload
Client->>Icmpv6Slice: payload_slice(type, payload_bytes)
Icmpv6Slice->>Icmpv6PayloadSlice: Icmpv6PayloadSlice::from_slice(type, payload)
Icmpv6PayloadSlice->>PayloadSliceType: dispatch to specific from_slice(payload)
PayloadSliceType->>PayloadSliceType: validate fixed part, parse fields
PayloadSliceType-->>Icmpv6PayloadSlice: return parsed variant
Icmpv6PayloadSlice-->>Client: Result<Icmpv6PayloadSlice>
alt iterate NDP options
Client->>PayloadSliceType: options_iterator()
PayloadSliceType->>NdpOptionsIterator: NdpOptionsIterator::from_slice(options_bytes)
NdpOptionsIterator->>NdpOptionsIterator: parse_next_option() -> NdpOptionSlice
NdpOptionsIterator-->>Client: iterator of NdpOptionSlice results
end
alt convert to owned
Client->>Icmpv6PayloadSlice: to_payload()
Icmpv6PayloadSlice->>OwnedPayload: construct owned Icmpv6Payload variant
OwnedPayload-->>Client: Icmpv6Payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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: 7
🧹 Nitpick comments (4)
etherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rs (1)
42-50: Consider validating the option type byte for defensive parsing.The
from_slicemethod validates the slice length but does not verify thatslice[0](the Type field) equalsNdpOptionType::PREFIX_INFORMATION(3). While the caller likely validates this upstream when dispatching, adding a defensive check here would catch misuse and provide clearer error messages.This is optional if the design intentionally relies on caller validation.
♻️ Optional: add type validation
pub fn from_slice(slice: &'a [u8]) -> Result<Self, NdpOptionReadError> { let slice: &'a [u8; PrefixInformation::LEN] = slice.try_into().map_err(|_| NdpOptionReadError::UnexpectedSize { option_id: NdpOptionType::PREFIX_INFORMATION, expected_size: PrefixInformation::LEN, actual_size: slice.len(), })?; + if slice[0] != NdpOptionType::PREFIX_INFORMATION.0 { + return Err(NdpOptionReadError::UnexpectedType { + expected: NdpOptionType::PREFIX_INFORMATION, + actual: NdpOptionType(slice[0]), + }); + } Ok(Self { slice }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rs` around lines 42 - 50, In from_slice (prefix_information_option_slice::from_slice) add a defensive check after validating length that slice[0] == NdpOptionType::PREFIX_INFORMATION as u8; if it does not match, return a descriptive Ndp option read error (e.g. add/return a NdpOptionReadError variant like InvalidType or UnexpectedType that includes the found and expected type) so callers get a clear error when the Type byte is wrong instead of silently accepting bad data.etherparse/src/transport/icmpv6/icmpv6_payload/mod.rs (2)
62-121: Good test coverage for inner payload types; consider adding tests forIcmpv6Payloadenum methods.The property-based tests thoroughly validate
to_bytes()for individual payload types. However, thelen()andwrite()methods onIcmpv6Payloaditself are not directly tested. Consider adding tests that exercise these enum-level methods to ensure the dispatch logic remains correct as the codebase evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/icmpv6_payload/mod.rs` around lines 62 - 121, Add unit/property tests that exercise the Icmpv6Payload enum-level API: call Icmpv6Payload::len() and Icmpv6Payload::write(&mut [u8]) (or the method names used in the enum) for each variant (e.g., Icmpv6Payload::RouterSolicitation, RouterAdvertisement, NeighborSolicitation, NeighborAdvertisement, Redirect) and assert that len() equals the expected byte length and that write() produces the same byte sequence as the corresponding inner-type to_bytes() result (or the expected byte arrays already used in the tests above); include both fixed-case tests and proptest cases that mirror the existing property-based inputs (reachable_time, retrans_timer, target_address, destination_address) to ensure dispatch logic remains correct.
32-43: Consider adding#[allow(clippy::len_without_is_empty)]or documenting the rationale.Clippy may flag
len()without a correspondingis_empty()method. Since all payload variants have a fixed non-zero length,is_empty()would always returnfalseand isn't semantically meaningful here. Adding an allow attribute or a brief doc comment explaining this would clarify intent.♻️ Optional: suppress clippy lint
impl Icmpv6Payload { /// Returns the serialized payload length in bytes. + #[allow(clippy::len_without_is_empty)] pub fn len(&self) -> usize {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/icmpv6_payload/mod.rs` around lines 32 - 43, Clippy will warn about having a len() method without is_empty(); update the Icmpv6Payload::len implementation to suppress or explain this by either adding #[allow(clippy::len_without_is_empty)] on the impl or adding a short doc comment on Icmpv6Payload::len stating that all variants (RouterSolicitation, RouterAdvertisement, NeighborSolicitation, NeighborAdvertisement, Redirect) have fixed non‑zero sizes so is_empty() would always be false; modify the impl block for Icmpv6Payload (and the len method) to include one of these changes to silence the lint and document intent.etherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rs (1)
129-276: Good property-based test coverage; consider adding tests forIcmpv6PayloadSliceenum methods.The tests thoroughly cover individual payload slice types and error cases. However, the enum-level methods (
from_slice,from_type_u8,slice,to_payload) are not directly tested. Consider adding tests for:
from_slicewith variousIcmpv6Typeinputsfrom_type_u8withcode_u8 != 0to verifyRawfallbackto_payloadreturningNoneforRawvariant🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rs` around lines 129 - 276, Add unit/property tests covering the enum-level API of Icmpv6PayloadSlice: call Icmpv6PayloadSlice::from_slice on buffers whose first byte corresponds to different Icmpv6Type values and assert the returned variant and slice() contents; test Icmpv6PayloadSlice::from_type_u8 with a nonzero code_u8 to confirm it yields the Raw variant; assert that to_payload() returns Some(...) for known payload variants and returns None for Icmpv6PayloadSlice::Raw; include checks that slice() returns the original byte slice for each variant and that from_slice correctly delegates to RouterSolicitationPayloadSlice, RouterAdvertisementPayloadSlice, NeighborSolicitationPayloadSlice, NeighborAdvertisementPayloadSlice, and RedirectPayloadSlice as appropriate.
🤖 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/icmpv6_type.rs`:
- Around line 699-706: payload_from_slice currently calls
payload_slice(...).to_payload() which for ND messages (e.g.,
RouterSolicitationPayload, NeighborSolicitationPayload) discards options and
prevents round-tripping; either update the owned payload construction or make
the API explicit: either (A) change the various to_payload implementations (look
for to_payload in icmpv6_payload_slice/*, especially
router_solicitation_payload_slice.rs and neighbor_solicitation_payload_slice.rs)
to capture and store trailing NDP options into the owned types so
payload_from_slice returns a full copy, or (B) alter
payload_from_slice/payload_slice names and docs to indicate “fixed-part-only”
behavior (and adjust return type/messages) so callers know options are
dropped—pick one approach and apply consistently across payload_from_slice,
payload_slice, and the RouterSolicitationPayload/NeighborSolicitationPayload
constructors.
In `@etherparse/src/transport/icmpv6/ndp_option_read_error.rs`:
- Around line 40-43: The Display implementation for the NDP option error variant
UnexpectedSize prints a malformed message ending with ").,": update the write!
format string in the UnexpectedSize arm so it ends cleanly (e.g. remove the
extra comma) and still interpolates option_id.0, expected_size and actual_size;
ensure the write! call within the UnexpectedSize match arm keeps the same
variables (option_id, expected_size, actual_size) but uses a corrected format
string like "... had unexpected size value {actual_size} (expected
{expected_size})." to eliminate the stray punctuation.
In `@etherparse/src/transport/icmpv6/ndp_option/prefix_information.rs`:
- Around line 33-67: from_bytes currently accepts any 32-byte buffer and
silently normalizes malformed headers on round-trip; change
PrefixInformation::from_bytes (and thus from_slice) to validate the on-wire
option header bytes before decoding: check that the first two bytes equal the
canonical ICMPv6 NDP prefix information values (type == 3 and length == 4) and
return an error (or Result) when they do not, or alternatively document both
functions as requiring a pre-validated Prefix Information option; update the
signature/return path of from_bytes/from_slice consistently (or add a new
try_from_bytes returning Result) so invalid headers are rejected rather than
normalized, and mirror the same validation for the analogous code mentioned at
lines 81-82.
In
`@etherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rs`:
- Around line 53-55: Update the doc comment for redirected_packet() to warn that
the returned slice (&self.slice[Self::FIXED_PART_LEN..]) may include trailing
zero-padding because the option length is encoded in 8-octet units; state that
callers must not assume all bytes are meaningful packet data and should trim or
parse the embedded packet using its own length fields (or use a helper that
strips padding) before interpreting payload bytes.
In
`@etherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rs`:
- Around line 2-3: from_slice currently stores the entire input buffer; change
it to parse the 2-byte NDP option header (use the same header parsing logic that
reads the type and length), reject length == 0 by returning NdpOptionReadError,
compute the exact header.byte_len() in bytes, verify the provided slice is at
least that long, and trim/keep only the header.byte_len() window when
constructing the SourceLinkLayerAddressOptionSlice so link_layer_address() only
sees the encoded option bytes; apply the same change to the other similar
constructors (lines referenced in the comment).
In
`@etherparse/src/transport/icmpv6/ndp_option/target_link_layer_address_option_slice.rs`:
- Around line 23-33: from_slice currently only checks for the 2-byte header and
accepts any slice >= NDP_OPTION_HEADER_LEN, allowing truncated options; change
from_slice to validate the advertised option length field (the second byte in
the header) — ensure the length byte is non-zero, compute advertised_bytes =
(slice[1] as usize) * 8, and verify slice.len() >= advertised_bytes (and
advertised_bytes >= NDP_OPTION_HEADER_LEN) before returning Ok. If the check
fails, return NdpOptionReadError::UnexpectedSize with option_id
NdpOptionType::TARGET_LINK_LAYER_ADDRESS, expected_size set to advertised_bytes
(or NDP_OPTION_HEADER_LEN if invalid zero), and actual_size set to slice.len();
keep references to from_slice, link_layer_address, NDP_OPTION_HEADER_LEN, and
NdpOptionType::TARGET_LINK_LAYER_ADDRESS.
In `@etherparse/src/transport/icmpv6/ndp_option/unknown_ndp_option_slice.rs`:
- Around line 2-3: The constructor currently keeps the caller-provided tail
slice before parsing the NDP option header, which allows a zero Length or
swallowing of subsequent options; update the constructor in
unknown_ndp_option_slice (e.g., UnknownNdpOptionSlice::new) to first parse the
option header (use NdpOptionType and the header/Length parsing helper), validate
that Length != 0 (return NdpOptionReadError on zero), compute the byte length
via header.byte_len(), and then store only &slice[..header.byte_len()] instead
of the full tail so the unknown option contains exactly its on-wire bytes.
---
Nitpick comments:
In `@etherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rs`:
- Around line 129-276: Add unit/property tests covering the enum-level API of
Icmpv6PayloadSlice: call Icmpv6PayloadSlice::from_slice on buffers whose first
byte corresponds to different Icmpv6Type values and assert the returned variant
and slice() contents; test Icmpv6PayloadSlice::from_type_u8 with a nonzero
code_u8 to confirm it yields the Raw variant; assert that to_payload() returns
Some(...) for known payload variants and returns None for
Icmpv6PayloadSlice::Raw; include checks that slice() returns the original byte
slice for each variant and that from_slice correctly delegates to
RouterSolicitationPayloadSlice, RouterAdvertisementPayloadSlice,
NeighborSolicitationPayloadSlice, NeighborAdvertisementPayloadSlice, and
RedirectPayloadSlice as appropriate.
In `@etherparse/src/transport/icmpv6/icmpv6_payload/mod.rs`:
- Around line 62-121: Add unit/property tests that exercise the Icmpv6Payload
enum-level API: call Icmpv6Payload::len() and Icmpv6Payload::write(&mut [u8])
(or the method names used in the enum) for each variant (e.g.,
Icmpv6Payload::RouterSolicitation, RouterAdvertisement, NeighborSolicitation,
NeighborAdvertisement, Redirect) and assert that len() equals the expected byte
length and that write() produces the same byte sequence as the corresponding
inner-type to_bytes() result (or the expected byte arrays already used in the
tests above); include both fixed-case tests and proptest cases that mirror the
existing property-based inputs (reachable_time, retrans_timer, target_address,
destination_address) to ensure dispatch logic remains correct.
- Around line 32-43: Clippy will warn about having a len() method without
is_empty(); update the Icmpv6Payload::len implementation to suppress or explain
this by either adding #[allow(clippy::len_without_is_empty)] on the impl or
adding a short doc comment on Icmpv6Payload::len stating that all variants
(RouterSolicitation, RouterAdvertisement, NeighborSolicitation,
NeighborAdvertisement, Redirect) have fixed non‑zero sizes so is_empty() would
always be false; modify the impl block for Icmpv6Payload (and the len method) to
include one of these changes to silence the lint and document intent.
In
`@etherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rs`:
- Around line 42-50: In from_slice (prefix_information_option_slice::from_slice)
add a defensive check after validating length that slice[0] ==
NdpOptionType::PREFIX_INFORMATION as u8; if it does not match, return a
descriptive Ndp option read error (e.g. add/return a NdpOptionReadError variant
like InvalidType or UnexpectedType that includes the found and expected type) so
callers get a clear error when the Type byte is wrong instead of silently
accepting bad data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13a4a402-8803-48e5-acde-b95352535409
📒 Files selected for processing (28)
etherparse/src/transport/icmpv4_header.rsetherparse/src/transport/icmpv6/icmpv6_payload/mod.rsetherparse/src/transport/icmpv6/icmpv6_payload/neighbor_advertisement_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/neighbor_solicitation_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/redirect_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/router_advertisement_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/router_solicitation_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/neighbor_advertisement_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/neighbor_solicitation_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/redirect_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/router_advertisement_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/router_solicitation_payload_slice.rsetherparse/src/transport/icmpv6/mod.rsetherparse/src/transport/icmpv6/ndp_option/mod.rsetherparse/src/transport/icmpv6/ndp_option/mtu_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/ndp_option_header.rsetherparse/src/transport/icmpv6/ndp_option/prefix_information.rsetherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/target_link_layer_address_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/unknown_ndp_option_slice.rsetherparse/src/transport/icmpv6/ndp_option_read_error.rsetherparse/src/transport/icmpv6/ndp_option_type_impl.rsetherparse/src/transport/icmpv6/ndp_options_iterator.rsetherparse/src/transport/icmpv6_slice.rsetherparse/src/transport/icmpv6_type.rs
etherparse/src/transport/icmpv6/ndp_option/prefix_information.rs
Outdated
Show resolved
Hide resolved
etherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rs
Show resolved
Hide resolved
etherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rs
Outdated
Show resolved
Hide resolved
etherparse/src/transport/icmpv6/ndp_option/target_link_layer_address_option_slice.rs
Show resolved
Hide resolved
etherparse/src/transport/icmpv6/ndp_option/unknown_ndp_option_slice.rs
Outdated
Show resolved
Hide resolved
xyzzyz
force-pushed
the
amichalik/neighbor-discovery
branch
from
8d719e9 to
1a1c431
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
etherparse/src/transport/icmpv6_slice.rs (1)
669-785: Add directicmp_type()coverage for the new ND message kinds.These cases validate
payload_slice(), but they never hit the newicmp_type()branches for Router Solicitation, Router Advertisement, or Redirect. A regression in that match would still pass this test suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6_slice.rs` around lines 669 - 785, Add explicit assertions that call Icmpv6Slice::from_slice(...).unwrap().icmp_type() for the new ND message kinds (TYPE_ROUTER_SOLICITATION, TYPE_ROUTER_ADVERTISEMENT, TYPE_REDIRECT_MESSAGE) in the payload_slice() test (or a new test) so the icmp_type() match arms are exercised; for each packet used in the existing Router Solicitation, Router Advertisement, and Redirect cases, call icmp_type() and assert it returns the expected Icmpv6Type/enum variant (or constant) corresponding to those message kinds to ensure regressions in icmp_type() are caught.etherparse/src/transport/icmpv6_type.rs (1)
699-732: Clarify that owned payloads contain only the fixed part.The method returns
(Icmpv6Payload, &[u8])where the owned payload holds the fixed-part fields (e.g.,target_address) and the second element contains trailing options. This is a valid design, but the doc comment could be more explicit thatIcmpv6Payloadvariants intentionally exclude NDP options.Consider adding a note like: "The returned
Icmpv6Payloadcontains only the fixed fields; NDP options must be parsed separately from the returned byte slice."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6_type.rs` around lines 699 - 732, The doc comment for payload_from_slice should explicitly state that the returned icmpv6::Icmpv6Payload holds only the fixed-part fields (e.g., target_address) and does not include NDP/option data; any trailing NDP options are returned as the second tuple element (&[u8]) and must be parsed separately. Update the comment above pub fn payload_from_slice to mention Icmpv6Payload variants intentionally exclude NDP options and that the caller should parse the returned byte slice for options (reference types: payload_from_slice, icmpv6::Icmpv6Payload, Icmpv6PayloadSlice).etherparse/src/transport/icmpv6/ndp_option/mod.rs (1)
21-24: Minor:prefix_informationmodule declaration placement.The
pub mod prefix_information;declaration on line 22 appears betweenmod unknown_ndp_option_slice;(line 21) and itspub use(line 24). Consider grouping the declaration/re-export pairs consistently for readability.♻️ Suggested reordering
mod unknown_ndp_option_slice; -pub mod prefix_information; - pub use unknown_ndp_option_slice::*; + +pub mod prefix_information;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/ndp_option/mod.rs` around lines 21 - 24, Reorder the module declarations so related declaration/re-export pairs are grouped: move the `pub mod prefix_information;` to be adjacent to its own `pub use` (or alternatively move the `pub use unknown_ndp_option_slice::*;` next to `mod unknown_ndp_option_slice;`) so that `unknown_ndp_option_slice`'s mod declaration and `pub use unknown_ndp_option_slice::*;` remain together and `prefix_information` is declared as its own grouped pair for readability.
🤖 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/icmpv6/icmpv6_payload_slice/router_solicitation_payload_slice.rs`:
- Around line 50-53: The to_payload() implementation on
RouterSolicitationPayloadSlice is currently returning an empty
RouterSolicitationPayload and discarding all ND options; update it so the owned
RouterSolicitationPayload includes the parsed options (e.g., a Vec of the ND
option type used by this module such as NdpOption/NeighborDiscoveryOption) and
have RouterSolicitationPayloadSlice::to_payload clone/collect those options into
that Vec (or, alternatively, rename to_payload to to_fixed_part() and document
that it only preserves the fixed header fields). Specifically modify the
RouterSolicitationPayload struct to carry an options collection, update
to_payload() to populate that collection from the slice's option
iterator/fields, and adjust any constructors/uses of RouterSolicitationPayload
to handle the new options field so the conversion is lossless.
---
Nitpick comments:
In `@etherparse/src/transport/icmpv6_slice.rs`:
- Around line 669-785: Add explicit assertions that call
Icmpv6Slice::from_slice(...).unwrap().icmp_type() for the new ND message kinds
(TYPE_ROUTER_SOLICITATION, TYPE_ROUTER_ADVERTISEMENT, TYPE_REDIRECT_MESSAGE) in
the payload_slice() test (or a new test) so the icmp_type() match arms are
exercised; for each packet used in the existing Router Solicitation, Router
Advertisement, and Redirect cases, call icmp_type() and assert it returns the
expected Icmpv6Type/enum variant (or constant) corresponding to those message
kinds to ensure regressions in icmp_type() are caught.
In `@etherparse/src/transport/icmpv6_type.rs`:
- Around line 699-732: The doc comment for payload_from_slice should explicitly
state that the returned icmpv6::Icmpv6Payload holds only the fixed-part fields
(e.g., target_address) and does not include NDP/option data; any trailing NDP
options are returned as the second tuple element (&[u8]) and must be parsed
separately. Update the comment above pub fn payload_from_slice to mention
Icmpv6Payload variants intentionally exclude NDP options and that the caller
should parse the returned byte slice for options (reference types:
payload_from_slice, icmpv6::Icmpv6Payload, Icmpv6PayloadSlice).
In `@etherparse/src/transport/icmpv6/ndp_option/mod.rs`:
- Around line 21-24: Reorder the module declarations so related
declaration/re-export pairs are grouped: move the `pub mod prefix_information;`
to be adjacent to its own `pub use` (or alternatively move the `pub use
unknown_ndp_option_slice::*;` next to `mod unknown_ndp_option_slice;`) so that
`unknown_ndp_option_slice`'s mod declaration and `pub use
unknown_ndp_option_slice::*;` remain together and `prefix_information` is
declared as its own grouped pair for readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8308d559-2c8d-4e65-b539-272b2e0073c5
📒 Files selected for processing (28)
etherparse/src/transport/icmpv4_header.rsetherparse/src/transport/icmpv6/icmpv6_payload/mod.rsetherparse/src/transport/icmpv6/icmpv6_payload/neighbor_advertisement_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/neighbor_solicitation_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/redirect_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/router_advertisement_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/router_solicitation_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/neighbor_advertisement_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/neighbor_solicitation_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/redirect_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/router_advertisement_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/router_solicitation_payload_slice.rsetherparse/src/transport/icmpv6/mod.rsetherparse/src/transport/icmpv6/ndp_option/mod.rsetherparse/src/transport/icmpv6/ndp_option/mtu_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/ndp_option_header.rsetherparse/src/transport/icmpv6/ndp_option/prefix_information.rsetherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/target_link_layer_address_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/unknown_ndp_option_slice.rsetherparse/src/transport/icmpv6/ndp_option_read_error.rsetherparse/src/transport/icmpv6/ndp_option_type_impl.rsetherparse/src/transport/icmpv6/ndp_options_iterator.rsetherparse/src/transport/icmpv6_slice.rsetherparse/src/transport/icmpv6_type.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- etherparse/src/transport/icmpv6/icmpv6_payload/router_solicitation_payload.rs
- etherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rs
- etherparse/src/transport/icmpv6/icmpv6_payload/neighbor_advertisement_payload.rs
- etherparse/src/transport/icmpv6/icmpv6_payload/mod.rs
etherparse/src/transport/icmpv6/icmpv6_payload_slice/router_solicitation_payload_slice.rs
Outdated
Show resolved
Hide resolved
Introduce structured ICMPv6 Neighbor Discovery modeling in both owned and borrowed forms. Add Icmpv6Payload/Icmpv6PayloadSlice variants for Router Solicitation, Router Advertisement, Neighbor Solicitation, Neighbor Advertisement, and Redirect messages, with per-message modules, fixed-part parsing and serialization, IPv6 address accessors, and conversions between slice and owned representations. Unsupported ICMPv6 message payloads remain representable as raw bytes. Add a full ND option layer under icmpv6::ndp_option: typed option slices (Source/Target Link-Layer Address, Prefix Information, Redirected Header, MTU, Unknown), NdpOptionType, NdpOptionHeader, PrefixInformation helpers, and NdpOptionsIterator with consistent size validation and stop-on-first-error behavior. Update ICMPv6 decoding integration in icmpv6_slice/icmpv6_type, include RFC 4861 packet layout docs, and expand module tests for payload and option parsing.
xyzzyz
force-pushed
the
amichalik/neighbor-discovery
branch
from
1a1c431 to
7783d39
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
etherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rs (1)
63-66: Document thatlink_layer_address()returns the raw padded option body.Like
redirected_packet()inetherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rsat Lines 76-82, this accessor exposes every byte after the 2-byte option header. Since the option length is encoded in 8-octet units, some link-layer types can leave trailing pad bytes in that region, so a short note would help callers avoid treating every returned byte as address material.Proposed doc tweak
- /// Returns the link-layer address bytes carried by the option. + /// Returns the bytes carried after the 2-byte option header. + /// + /// Note that this may include trailing zero padding, because the option + /// length is encoded in units of 8 octets and the actual address length + /// depends on the underlying link-layer type. pub fn link_layer_address(&self) -> &'a [u8] { &self.slice[NDP_OPTION_HEADER_LEN..] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rs` around lines 63 - 66, Update the doc comment for the method link_layer_address() on SourceLinkLayerAddressOptionSlice to state that it returns the raw padded option body (every byte after the 2-byte option header) and may include trailing pad bytes because option length is encoded in 8-octet units; advise callers to trim or interpret returned bytes according to the option length/type rather than assuming every returned byte is address material.etherparse/src/transport/icmpv6/ndp_option_read_error.rs (1)
20-26: Split type mismatches from length mismatches before this error shape hardens.
UnexpectedHeaderalways requires anexpected_length_units, but variable-length options do not have a single expected value. The new callers inetherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rsat Lines 27-32 andetherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rsat Lines 42-47 already have to echo the actual length back into both fields, so the resulting message can claim “expected length X, got X” on a pure type mismatch. A dedicatedUnexpectedTypevariant, or making the expected length optional, would keep these diagnostics truthful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/ndp_option_read_error.rs` around lines 20 - 26, The UnexpectedHeader enum variant in ndp_option_read_error.rs conflates type vs length mismatches; change it so type and length errors are distinct—either replace UnexpectedHeader with two variants (e.g., UnexpectedType { expected: NdpOptionType, actual: NdpOptionType } and UnexpectedLength { option_id: NdpOptionType, expected_length_units: Option<u8>, actual_length_units: u8 }) or make expected_length_units an Option<u8> so callers can indicate "no single expected length." Update call sites in source_link_layer_address_option_slice.rs and redirected_header_option_slice.rs to emit the new UnexpectedType when only the type differs (and use UnexpectedLength or pass None for expected length when length is variable), and update any pattern matches or error formatting that destructures UnexpectedHeader to handle the new variants/optional field.etherparse/src/transport/icmpv6/mod.rs (1)
16-34: Minor: Consider reordering re-exports for consistency.The
pub use ndp_option::prefix_information::*;on line 16 appears before themod ndp_option;declaration on line 21. While this compiles correctly (Rust resolves module declarations before use statements), it's unconventional compared to the other modules which follow themod X; pub use X::*;pattern.Consider moving line 16 after line 22 for consistency:
-pub use ndp_option::prefix_information::*; - mod ndp_option_type_impl; pub use ndp_option_type_impl::*; mod ndp_option; pub use ndp_option::*; +pub use ndp_option::prefix_information::*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/mod.rs` around lines 16 - 34, Move the re-export of ndp_option::prefix_information (the line using pub use ndp_option::prefix_information::*) to follow the module declaration for ndp_option (mod ndp_option;) so it matches the existing pattern used for other modules (mod X; pub use X::*;), i.e., place the pub use after mod ndp_option to maintain consistent ordering with the other exports.etherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rs (2)
255-297: Good boundary testing coverage.The
len_errorstest properly validates error handling for undersized slices at the exact boundary (FIXED_PART_LEN - 1) for each type that has fixed-part requirements.Consider adding tests for the
Icmpv6PayloadSliceenum methods (from_slice,from_type_u8,slice()) and theRawvariant in a future iteration to complete coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rs` around lines 255 - 297, Add unit tests covering the Icmpv6PayloadSlice enum and Raw variant: create tests that exercise Icmpv6PayloadSlice::from_slice, Icmpv6PayloadSlice::from_type_u8, and the slice() accessor for each concrete variant (RouterAdvertisementPayloadSlice, NeighborSolicitationPayloadSlice, NeighborAdvertisementPayloadSlice, RedirectPayloadSlice), asserting successful construction for exact FIXED_PART_LEN and larger lengths and that slice() returns the original input; also add tests that from_type_u8 returns the correct enum variant for known ICMPv6 type bytes and that the Raw variant round-trips (from_slice yields Raw for unknown/unsupported types and slice() returns its payload), and include negative assertions for undersized inputs (using each FIXED_PART_LEN - 1) to mirror existing len_errors behavior.
72-74: Consider idiomatic comparison order.The Yoda condition
0 != code_u8works but is non-idiomatic in Rust. The conventional form iscode_u8 != 0.♻️ Suggested change
- if 0 != code_u8 { + if code_u8 != 0 { return Ok(Icmpv6PayloadSlice::Raw(payload)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rs` around lines 72 - 74, Change the Yoda-style condition to the idiomatic Rust ordering: replace the comparison that currently checks "0 != code_u8" with "code_u8 != 0" where the variable code_u8 is used to decide returning Icmpv6PayloadSlice::Raw(payload); keep the same logic and return value, only invert the operands for standard style.
🤖 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/icmpv6/icmpv6_payload_slice/mod.rs`:
- Around line 255-297: Add unit tests covering the Icmpv6PayloadSlice enum and
Raw variant: create tests that exercise Icmpv6PayloadSlice::from_slice,
Icmpv6PayloadSlice::from_type_u8, and the slice() accessor for each concrete
variant (RouterAdvertisementPayloadSlice, NeighborSolicitationPayloadSlice,
NeighborAdvertisementPayloadSlice, RedirectPayloadSlice), asserting successful
construction for exact FIXED_PART_LEN and larger lengths and that slice()
returns the original input; also add tests that from_type_u8 returns the correct
enum variant for known ICMPv6 type bytes and that the Raw variant round-trips
(from_slice yields Raw for unknown/unsupported types and slice() returns its
payload), and include negative assertions for undersized inputs (using each
FIXED_PART_LEN - 1) to mirror existing len_errors behavior.
- Around line 72-74: Change the Yoda-style condition to the idiomatic Rust
ordering: replace the comparison that currently checks "0 != code_u8" with
"code_u8 != 0" where the variable code_u8 is used to decide returning
Icmpv6PayloadSlice::Raw(payload); keep the same logic and return value, only
invert the operands for standard style.
In `@etherparse/src/transport/icmpv6/mod.rs`:
- Around line 16-34: Move the re-export of ndp_option::prefix_information (the
line using pub use ndp_option::prefix_information::*) to follow the module
declaration for ndp_option (mod ndp_option;) so it matches the existing pattern
used for other modules (mod X; pub use X::*;), i.e., place the pub use after mod
ndp_option to maintain consistent ordering with the other exports.
In `@etherparse/src/transport/icmpv6/ndp_option_read_error.rs`:
- Around line 20-26: The UnexpectedHeader enum variant in
ndp_option_read_error.rs conflates type vs length mismatches; change it so type
and length errors are distinct—either replace UnexpectedHeader with two variants
(e.g., UnexpectedType { expected: NdpOptionType, actual: NdpOptionType } and
UnexpectedLength { option_id: NdpOptionType, expected_length_units: Option<u8>,
actual_length_units: u8 }) or make expected_length_units an Option<u8> so
callers can indicate "no single expected length." Update call sites in
source_link_layer_address_option_slice.rs and redirected_header_option_slice.rs
to emit the new UnexpectedType when only the type differs (and use
UnexpectedLength or pass None for expected length when length is variable), and
update any pattern matches or error formatting that destructures
UnexpectedHeader to handle the new variants/optional field.
In
`@etherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rs`:
- Around line 63-66: Update the doc comment for the method link_layer_address()
on SourceLinkLayerAddressOptionSlice to state that it returns the raw padded
option body (every byte after the 2-byte option header) and may include trailing
pad bytes because option length is encoded in 8-octet units; advise callers to
trim or interpret returned bytes according to the option length/type rather than
assuming every returned byte is address material.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeb1fbd2-23e0-44f9-9353-b58ba574bc11
📒 Files selected for processing (28)
etherparse/src/transport/icmpv4_header.rsetherparse/src/transport/icmpv6/icmpv6_payload/mod.rsetherparse/src/transport/icmpv6/icmpv6_payload/neighbor_advertisement_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/neighbor_solicitation_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/redirect_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/router_advertisement_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload/router_solicitation_payload.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/mod.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/neighbor_advertisement_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/neighbor_solicitation_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/redirect_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/router_advertisement_payload_slice.rsetherparse/src/transport/icmpv6/icmpv6_payload_slice/router_solicitation_payload_slice.rsetherparse/src/transport/icmpv6/mod.rsetherparse/src/transport/icmpv6/ndp_option/mod.rsetherparse/src/transport/icmpv6/ndp_option/mtu_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/ndp_option_header.rsetherparse/src/transport/icmpv6/ndp_option/prefix_information.rsetherparse/src/transport/icmpv6/ndp_option/prefix_information_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/redirected_header_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/source_link_layer_address_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/target_link_layer_address_option_slice.rsetherparse/src/transport/icmpv6/ndp_option/unknown_ndp_option_slice.rsetherparse/src/transport/icmpv6/ndp_option_read_error.rsetherparse/src/transport/icmpv6/ndp_option_type_impl.rsetherparse/src/transport/icmpv6/ndp_options_iterator.rsetherparse/src/transport/icmpv6_slice.rsetherparse/src/transport/icmpv6_type.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- etherparse/src/transport/icmpv6/icmpv6_payload/router_solicitation_payload.rs
- etherparse/src/transport/icmpv6/icmpv6_payload/neighbor_advertisement_payload.rs
- etherparse/src/transport/icmpv6/icmpv6_payload/router_advertisement_payload.rs
- etherparse/src/transport/icmpv6/ndp_option/unknown_ndp_option_slice.rs
- etherparse/src/transport/icmpv6_slice.rs
- etherparse/src/transport/icmpv6/icmpv6_payload_slice/redirect_payload_slice.rs
- etherparse/src/transport/icmpv6/ndp_option/target_link_layer_address_option_slice.rs
- etherparse/src/transport/icmpv6/icmpv6_payload/mod.rs
- etherparse/src/transport/icmpv6/icmpv6_payload/neighbor_solicitation_payload.rs
- etherparse/src/transport/icmpv6/ndp_option/prefix_information.rs
JulianSchmid
reviewed
Mar 8, 2026
Owner
JulianSchmid
left a comment
JulianSchmid
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR. But I will need some time to go through it all and review it 😅
Contributor
Author
|
Heh, sorry about this! I was thinking about how to make it smaller, but I was worried that if I don't sketch out the entire thing up front, I'd misdesign something in a way that would require breaking the API compatibility later. In fact, the AI reviewer actually pointed out something that required changing the API ( At this point, though, I could try to split it into multiple PRs: for example, I could make the PayloadSlice PR first, then the owned payloads, and then the options. Would you like me to do that? FWIW, I plan to also implement writing all these payloads using the PacketBuilder, but that definitely can come in a future separate PR. |
Owner
|
No worries and no action needed from your side. I will finish up this PR. So far I did not see any issues 👍. But thanks for the offer. |
JulianSchmid
approved these changes
Mar 8, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #140 +/- ##
=============================
=============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Thanks! |
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.
Introduce structured ICMPv6 Neighbor Discovery modeling in both owned and borrowed forms. Add Icmpv6Payload/Icmpv6PayloadSlice variants for Router Solicitation, Router Advertisement, Neighbor Solicitation, Neighbor Advertisement, and Redirect messages, with per-message modules, fixed-part parsing and serialization, IPv6 address accessors, and conversions between slice and owned representations. Unsupported ICMPv6 message payloads remain representable as raw bytes.
Add a full ND option layer under icmpv6::ndp_option: typed option slices (Source/Target Link-Layer Address, Prefix Information, Redirected Header, MTU, Unknown), NdpOptionType, NdpOptionHeader, PrefixInformation helpers, and NdpOptionsIterator with consistent size validation and stop-on-first-error behavior. Update ICMPv6 decoding integration in icmpv6_slice/icmpv6_type, include RFC 4861 packet layout docs, and expand module tests for payload and option parsing.
Summary by CodeRabbit
New Features
Tests