Enable PacketBuilder in no_std (alloc/slice) while preserving std API#135
Merged
JulianSchmid merged 2 commits intoJulianSchmid:masterfrom Apr 20, 2026
Merged
Conversation
Contributor
Author
|
Hey, is there any chance this can get merged? Thanks! |
Owner
|
I will try to make some time this weekend. Can you rebase the PR before that? I see a merge conflict in |
xyzzyz
force-pushed
the
amichalik/packet-builder-no-std
branch
from
5b9dabd to
8ec775c
Compare
📝 WalkthroughWalkthroughAdds a crate-internal CoreWrite abstraction and writer implementations, introduces slice/vec-specific packet build error types, refactors IPv4/IPv6 extension writers to use internal generic write paths, exposes packet_builder unconditionally, and adds an explicit Changes
Sequence DiagramsequenceDiagram
participant User
participant PacketBuilder
participant CoreWrite as CoreWrite Impl
participant Writer as Writer (IoWriter/VecWriter/SliceCoreWrite)
participant Error
User->>PacketBuilder: write_to_vec / write_to_slice / write
PacketBuilder->>PacketBuilder: final_write_with_net<W,E>
PacketBuilder->>CoreWrite: call write_internal (IPv4/IPv6 ext, headers)
CoreWrite->>CoreWrite: header.to_bytes()
CoreWrite->>Writer: write_all(&[u8])
alt Success
Writer-->>CoreWrite: Ok
CoreWrite-->>PacketBuilder: Ok
PacketBuilder-->>User: Ok (bytes/count)
else IO or Content error
Writer-->>CoreWrite: Err(WriteError::Io / WriteError::Content)
CoreWrite-->>PacketBuilder: Err(WriteError)
PacketBuilder->>Error: map Into Build*WriteError via From
Error-->>User: Err(BuildSliceWriteError / BuildVecWriteError / BuildWriteError)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
Contributor
Author
|
Rebased. |
![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/packet/build_slice_write_error.rs`:
- Around line 14-17: The doc comment for the enum variant Ipv4Exts is incomplete
and the human-facing message for the ICMPv6/IPv4 case has a grammar issue;
update the Ipv4Exts doc to a complete sentence (e.g., "Error when IPv4
extensions cannot be serialized due to internal consistency errors (e.g., an
inconsistent header).") and fix the Display text in the relevant fmt
implementation for BuildSliceWriteError (or the Display impl that formats the
ICMPv6/IPv4 case) to use correct grammar/wording such as "failed to serialize
ICMPv6/IPv4 message" or "could not serialize ICMPv6/IPv4 message" and ensure
"can not" is changed to "cannot" where applicable.
In `@etherparse/src/err/packet/build_vec_write_error.rs`:
- Around line 4-6: Add the docsrs cfg annotation to the public alloc-only error
type by applying #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] to the
BuildVecWriteError enum so the docs show the "alloc" feature badge like other
alloc-gated items; locate the pub enum BuildVecWriteError declaration and add
that cfg_attr above it (same pattern used for BuildWriteError) to ensure
consistent docsrs feature visibility.
In `@etherparse/src/packet_builder.rs`:
- Around line 2492-2564: The test module white_box_tests uses alloc (see use
alloc::vec::Vec;) but is only gated by #[cfg(test)], causing compilation
failures when running cargo test --no-default-features; either add an explicit
cfg gate to the module (e.g. #[cfg(all(test, feature = "alloc"))] on the
white_box_tests module) so the tests only compile when the alloc feature is
enabled, or update CI to run an additional job/step that executes cargo test
--no-default-features --features alloc to validate this feature combination (and
ensure cargo test --no-default-features indeed runs tests rather than just
compiling).
🪄 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: e1ce72d4-0bab-4fec-b20e-ddd2173aba7e
📒 Files selected for processing (10)
etherparse/Cargo.tomletherparse/src/err/packet/build_slice_write_error.rsetherparse/src/err/packet/build_vec_write_error.rsetherparse/src/err/packet/build_write_error.rsetherparse/src/err/packet/mod.rsetherparse/src/lib.rsetherparse/src/net/ipv4_exts.rsetherparse/src/net/ipv6_exts.rsetherparse/src/packet_builder.rsetherparse/src/writer.rs
Comment on lines
+14
to
+17
| /// Error if the IPv4 extensions can not be serialized | ||
| /// because of internal consistency errors (i.e. a header | ||
| /// is never). | ||
| Ipv4Exts(ipv4_exts::ExtsWalkError), |
There was a problem hiding this comment.
Clean up the new public error text.
The IPv4 extension docs end with an incomplete sentence, and the ICMPv6/IPv4 display message has a grammar issue.
📝 Proposed wording cleanup
- /// because of internal consistency errors (i.e. a header
- /// is never).
+ /// because of internal consistency errors.
...
- "Error: ICMPv6 can not be combined with an IPv4 headers (checksum can not be calculated)."
+ "Error: ICMPv6 can not be combined with an IPv4 header (checksum can not be calculated)."Also applies to: 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etherparse/src/err/packet/build_slice_write_error.rs` around lines 14 - 17,
The doc comment for the enum variant Ipv4Exts is incomplete and the human-facing
message for the ICMPv6/IPv4 case has a grammar issue; update the Ipv4Exts doc to
a complete sentence (e.g., "Error when IPv4 extensions cannot be serialized due
to internal consistency errors (e.g., an inconsistent header).") and fix the
Display text in the relevant fmt implementation for BuildSliceWriteError (or the
Display impl that formats the ICMPv6/IPv4 case) to use correct grammar/wording
such as "failed to serialize ICMPv6/IPv4 message" or "could not serialize
ICMPv6/IPv4 message" and ensure "can not" is changed to "cannot" where
applicable.
PacketBuilder serialization is split from std::io::Write so packet building works in no_std environments while keeping std behavior intact. Key changes - Introduce an internal writer abstraction (`CoreWrite`) and internal `WriteError<IO, Content>` in `src/writer.rs`. - Add an explicit `alloc` feature and make `std` depend on `alloc`. - Make `PacketBuilder` available without `std`. - Keep std `write(...) -> BuildWriteError` API, with `BuildWriteError` remaining std-only. - Add no_std-capable write APIs: - `write_to_vec` (alloc) with `BuildVecWriteError` - `write_to_slice` (no alloc) with `BuildSliceWriteError` - Refactor IPv4/IPv6 extension writing through shared internal methods used by both std and no_std paths. - Add docsrs cfg annotations for alloc-only APIs. - Add focused tests for vec and slice write paths: - write to empty vec - write to pre-populated vec - write to slice success path - write to slice too-small error path Compatibility - std users keep existing `PacketBuilder::write` behavior. - no_std users gain packet serialization through vec/slice writers.
xyzzyz
force-pushed
the
amichalik/packet-builder-no-std
branch
from
8ec775c to
a4a058d
Compare
JulianSchmid
approved these changes
Apr 19, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
etherparse/src/packet_builder.rs (2)
42-55: Prefer returning the writer's actual position over the precomputedrequired.
final_write_to_slicereturnsOk(required)assumingfinal_sizeis exact. It almost certainly is today, but coupling the reported byte count to a separate size predictor is a hidden invariant that will silently return a wrong count iffinal_sizeever drifts from the write path (or if a caller customizes one without the other). Tracking the writer'sposmakes the return self-consistent and removes the need for the invariant.♻️ Proposed change
In
etherparse/src/writer.rs, expose the current position:impl<'a> SliceCoreWrite<'a> { #[inline] pub(crate) fn new(buf: &'a mut [u8]) -> Self { SliceCoreWrite { buf, pos: 0 } } #[inline] pub(crate) fn pos(&self) -> usize { self.pos } }Then use it in
final_write_to_slice:let mut writer = SliceCoreWrite::new(slice); final_write_with_net::<_, _, BuildSliceWriteError>(builder, &mut writer, payload)?; - Ok(required) + Ok(writer.pos())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/packet_builder.rs` around lines 42 - 55, final_write_to_slice currently returns the precomputed required size from final_size, which can diverge from the actual bytes written; modify the code to return the writer's actual position instead: expose a pos() accessor on SliceCoreWrite (add pub(crate) fn pos(&self) -> usize to the SliceCoreWrite impl that is constructed by SliceCoreWrite::new), then in final_write_to_slice call final_write_with_net(..., &mut writer, ...) and return Ok(writer.pos()) rather than Ok(required); keep error handling with BuildSliceWriteError and other symbols (final_write_to_slice, final_size, final_write_with_net, SliceCoreWrite::new, SliceCoreWrite::pos) intact.
2492-2566: Nice coverage of the new vec/slice paths.Tests cover the four primary cases (empty vec, pre-populated vec preserved, slice success with sentinel bytes around the written region, and slice-too-small returning
Space(required)). One nit-level idea (optional): add a proptest-backed roundtrip that writes the same builder viawrite,write_to_vec, andwrite_to_sliceand asserts all three produce byte-identical output — this would lock in parity across the three backends for future refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@etherparse/src/packet_builder.rs` around lines 2492 - 2566, Add a property-based roundtrip test that, for varied payload sizes and random prefixes/slice offsets, constructs a PacketBuilder (use PacketBuilder::ipv4(...).udp(...)), then obtains bytes via write(&mut Vec), write_to_vec(&mut Vec) (starting from empty and from a random prefix), and write_to_slice(&mut [u8]) (with enough space determined by builder.size(payload.len())), and assert all produced packet bytes (excluding prefixes/sentinels) are identical and that write_to_slice returns the expected written length (builder.size). Use varying payload lengths and contents to lock parity between write, write_to_vec, and write_to_slice implementations.
🤖 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/packet/build_slice_write_error.rs`:
- Around line 36-71: The file contains unused From impls that convert
SliceWriteSpaceError (the direct impl From<SliceWriteSpaceError> for
BuildSliceWriteError and the two impls
From<crate::WriteError<SliceWriteSpaceError, ipv4_exts::ExtsWalkError>> and
From<crate::WriteError<SliceWriteSpaceError, ipv6_exts::ExtsWalkError>>), which
the current pipeline never invokes; either remove these three impl blocks (the
From<SliceWriteSpaceError> impl and the two WriteError->BuildSliceWriteError
impls) to avoid dead code, or add explicit code paths and tests that exercise
them (e.g., adapt a builder or header write path to return/propagate
BuildSliceWriteError via these conversions and add unit tests asserting the
conversion produces the expected BuildSliceWriteError variants). Ensure you
update or add tests to cover the chosen approach and remove any now-unnecessary
imports.
---
Nitpick comments:
In `@etherparse/src/packet_builder.rs`:
- Around line 42-55: final_write_to_slice currently returns the precomputed
required size from final_size, which can diverge from the actual bytes written;
modify the code to return the writer's actual position instead: expose a pos()
accessor on SliceCoreWrite (add pub(crate) fn pos(&self) -> usize to the
SliceCoreWrite impl that is constructed by SliceCoreWrite::new), then in
final_write_to_slice call final_write_with_net(..., &mut writer, ...) and return
Ok(writer.pos()) rather than Ok(required); keep error handling with
BuildSliceWriteError and other symbols (final_write_to_slice, final_size,
final_write_with_net, SliceCoreWrite::new, SliceCoreWrite::pos) intact.
- Around line 2492-2566: Add a property-based roundtrip test that, for varied
payload sizes and random prefixes/slice offsets, constructs a PacketBuilder (use
PacketBuilder::ipv4(...).udp(...)), then obtains bytes via write(&mut Vec),
write_to_vec(&mut Vec) (starting from empty and from a random prefix), and
write_to_slice(&mut [u8]) (with enough space determined by
builder.size(payload.len())), and assert all produced packet bytes (excluding
prefixes/sentinels) are identical and that write_to_slice returns the expected
written length (builder.size). Use varying payload lengths and contents to lock
parity between write, write_to_vec, and write_to_slice implementations.
🪄 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: fe5385a0-acb3-44b8-85d5-b80be8ebe7fe
📒 Files selected for processing (10)
etherparse/Cargo.tomletherparse/src/err/packet/build_slice_write_error.rsetherparse/src/err/packet/build_vec_write_error.rsetherparse/src/err/packet/build_write_error.rsetherparse/src/err/packet/mod.rsetherparse/src/lib.rsetherparse/src/net/ipv4_exts.rsetherparse/src/net/ipv6_exts.rsetherparse/src/packet_builder.rsetherparse/src/writer.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- etherparse/Cargo.toml
- etherparse/src/err/packet/mod.rs
- etherparse/src/err/packet/build_vec_write_error.rs
- etherparse/src/err/packet/build_write_error.rs
- etherparse/src/net/ipv6_exts.rs
Comment on lines
+36
to
+71
| impl From<SliceWriteSpaceError> for BuildSliceWriteError { | ||
| fn from(value: SliceWriteSpaceError) -> Self { | ||
| BuildSliceWriteError::Space(value.required_len) | ||
| } | ||
| } | ||
|
|
||
| impl From<super::TransportChecksumError> for BuildSliceWriteError { | ||
| fn from(value: super::TransportChecksumError) -> Self { | ||
| match value { | ||
| super::TransportChecksumError::PayloadLen(err) => BuildSliceWriteError::PayloadLen(err), | ||
| super::TransportChecksumError::Icmpv6InIpv4 => BuildSliceWriteError::Icmpv6InIpv4, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<crate::WriteError<SliceWriteSpaceError, ipv4_exts::ExtsWalkError>> | ||
| for BuildSliceWriteError | ||
| { | ||
| fn from(value: crate::WriteError<SliceWriteSpaceError, ipv4_exts::ExtsWalkError>) -> Self { | ||
| match value { | ||
| crate::WriteError::Io(err) => BuildSliceWriteError::Space(err.required_len), | ||
| crate::WriteError::Content(err) => BuildSliceWriteError::Ipv4Exts(err), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<crate::WriteError<SliceWriteSpaceError, ipv6_exts::ExtsWalkError>> | ||
| for BuildSliceWriteError | ||
| { | ||
| fn from(value: crate::WriteError<SliceWriteSpaceError, ipv6_exts::ExtsWalkError>) -> Self { | ||
| match value { | ||
| crate::WriteError::Io(err) => BuildSliceWriteError::Space(err.required_len), | ||
| crate::WriteError::Content(err) => BuildSliceWriteError::Ipv6Exts(err), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any code that constructs/returns BuildSliceWriteError from a SliceWriteSpaceError
# or from WriteError<SliceWriteSpaceError, _>.
rg -nP -C3 'SliceWriteSpaceError' --type=rust
echo '--- call sites returning BuildSliceWriteError ---'
rg -nP -C2 'BuildSliceWriteError' --type=rust
echo '--- SliceCoreWriteError usage ---'
rg -nP -C2 'SliceCoreWriteError' --type=rustRepository: JulianSchmid/etherparse
Length of output: 25197
🏁 Script executed:
# Search for places where SliceWriteSpaceError is converted to BuildSliceWriteError
# Look for .into() calls, ? operators, or explicit From::from calls
rg -n 'SliceWriteSpaceError' --type=rust -A 5 | grep -E '(\.into\(\)|\.from\(|BuildSliceWriteError)' -B 2 -A 1
# Also check if there are tests specifically for these From impls
fd -e rs --type f | xargs grep -l 'BuildSliceWriteError' | xargs grep -l 'SliceWriteSpaceError'
# Search for uses of the individual header write_to_slice methods
rg 'write_to_slice' --type=rust -B 2 -A 3Repository: JulianSchmid/etherparse
Length of output: 11984
🏁 Script executed:
# Check visibility and how BuildSliceWriteError is exported
rg -n 'pub.*BuildSliceWriteError' --type=rust
# Check the error enum structure and variant documentation
cat -n etherparse/src/err/packet/build_slice_write_error.rs | head -35
# Check if there's any documentation suggesting these conversions should be used
rg -B 3 'From<SliceWriteSpaceError>' etherparse/src/err/packet/build_slice_write_error.rs
# Look for any public API that might return these error types mixed
rg 'pub.*fn.*BuildSliceWriteError' --type=rust -A 2Repository: JulianSchmid/etherparse
Length of output: 2029
Remove unused From<SliceWriteSpaceError> impls or add code paths that use them.
The three From impls for SliceWriteSpaceError in this file (lines 36–40 and 51–71) have no reachable call sites. The active packet builder pipeline uses SliceCoreWriteError exclusively, and individual header write_to_slice() methods (e.g., ethernet2_header, linux_sll_header) return SliceWriteSpaceError directly rather than through BuildSliceWriteError. If these conversions are not needed, drop them; otherwise, add explicit code paths and tests that exercise them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@etherparse/src/err/packet/build_slice_write_error.rs` around lines 36 - 71,
The file contains unused From impls that convert SliceWriteSpaceError (the
direct impl From<SliceWriteSpaceError> for BuildSliceWriteError and the two
impls From<crate::WriteError<SliceWriteSpaceError, ipv4_exts::ExtsWalkError>>
and From<crate::WriteError<SliceWriteSpaceError, ipv6_exts::ExtsWalkError>>),
which the current pipeline never invokes; either remove these three impl blocks
(the From<SliceWriteSpaceError> impl and the two
WriteError->BuildSliceWriteError impls) to avoid dead code, or add explicit code
paths and tests that exercise them (e.g., adapt a builder or header write path
to return/propagate BuildSliceWriteError via these conversions and add unit
tests asserting the conversion produces the expected BuildSliceWriteError
variants). Ensure you update or add tests to cover the chosen approach and
remove any now-unnecessary imports.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #135 +/- ##
=============================
=============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
|
Thanks for the PR @xyzzyz , looked all good. I included it in the 0.20.0 & 0.20.1 release. |
Contributor
Author
|
Thank you! |
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.
PacketBuilder serialization is split from std::io::Write so packet building works in no_std environments while keeping std behavior intact.
Key changes
CoreWrite) and internalWriteError<IO, Content>insrc/writer.rs.allocfeature and makestddepend onalloc.PacketBuilderavailable withoutstd.write(...) -> BuildWriteErrorAPI, withBuildWriteErrorremaining std-only.write_to_vec(alloc) withBuildVecWriteErrorwrite_to_slice(no alloc) withBuildSliceWriteErrorCompatibility
PacketBuilder::writebehavior.Summary by CodeRabbit
New Features
Bug Fixes / Reliability