(8) feat(dpdk): test EAL harness (with_eal macro + const fn)#1572
Merged
Conversation
This was referenced May 31, 2026
daniel-noland
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds shared DPDK EAL initialization support for tests (via a start_eal() helper and a #[with_eal] proc-macro), and refactors an ACL helper to be usable in const contexts.
Changes:
- Introduces
dpdk::test_support::start_eal()(OnceLock-backed) to initialize EAL once per process with test-friendly flags. - Adds
dataplane-dpdk-test-macrosproviding#[with_eal], and wires it intodataplane-dpdkbehind atestfeature. - Refactors
AclBuildConfig::compute_min_input_sizeto aconst fnand adds a const-context unit test; updates ACL tests to use the macro and simplifies some call sites.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dpdk/src/test_support.rs | New shared EAL init helper for tests. |
| dpdk/src/lib.rs | Exposes test_support in tests / test feature; re-exports #[with_eal] under feature = "test". |
| dpdk/src/acl/mod.rs | Updates ACL unit tests to use #[with_eal]; minor doc/example tweak. |
| dpdk/src/acl/config.rs | Makes compute_min_input_size const-friendly and adds a const-context test. |
| dpdk/Cargo.toml | Adds test feature and optional deps/macro wiring for test harness support. |
| dpdk-test-macros/src/lib.rs | Implements the #[with_eal] attribute macro. |
| dpdk-test-macros/Cargo.toml | New proc-macro crate manifest. |
| Cargo.toml | Adds new workspace member + workspace dependency entry. |
| Cargo.lock | Locks new proc-macro crate and updated dependency graph. |
mvachhar
approved these changes
Jun 1, 2026
Member
|
Sign-off tag missing |
daniel-noland
force-pushed
the
pr/daniel-noland/threading-rewrite
branch
from
2021414 to
b7b5ece
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/dpdk-test-eal
branch
from
9da6fad to
14adb95
Compare
📝 WalkthroughWalkthroughThis PR adds five new workspace crates implementing a match-action packet matching system with fixed-size field serialization, generic lookup abstractions, procedural-macro-driven key derivation, and DPDK test infrastructure. ChangesMatch-action matching infrastructure with fixed-size serialization and DPDK test support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
daniel-noland
force-pushed
the
pr/daniel-noland/dpdk-test-eal
branch
5 times, most recently
from
9808e5a to
0e83d83
Compare
Lets callers derive the rte_acl input-buffer size requirement from a const FIELD_DEFS array at compile time -- useful for feeding into a const generic (e.g. the STRIDE on a DPDK-backed Lookup type alias) rather than rediscovering it at runtime. The runtime path through AclBuildConfig::new still calls into the same helper and caches the result; only the surface broadens from fn to const fn (with the for-loop rewritten as a while-let-i index walk that const-fn-eval can chew on). Existing FieldExtentOverflow validation is preserved; in a const context overflow becomes a compile error instead of UB. Adds one test exercising the const path: const MIN_INPUT_SIZE = AclBuildConfig::compute_min_input_size(&DEFS); plus the assertion that the runtime path returns the same value. just fmt; cargo check -p dataplane-dpdk --all-targets passes. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Introduces shared EAL test scaffolding for downstream crates that
need to exercise rte_acl, mempools, or any other DPDK runtime under
`#[test]`. Two pieces, both behind the new dpdk `test` feature so
production builds remain unchanged:
- dpdk-test-macros: proc-macro crate exposing #[with_eal], which
injects `let _eal = <dpdk_crate>::test_support::start_eal();` at
the top of a #[test] function. Resolves the dpdk crate's path via
proc-macro-crate so the macro works in-tree (`crate`), under the
workspace alias (`::dpdk`), or with the canonical name
(`::dataplane_dpdk`).
- dpdk::test_support: hosts a shared OnceLock<Eal> initialized with
--no-huge / --no-pci / --in-memory and the cpu-affinity-aware
--lcores derivation that was previously inline in acl/mod.rs.
Once-per-process by construction; safe under nextest's per-test
forking and single-process runners alike.
dpdk/src/acl/mod.rs picks up the new macro: every test in the module
loses its `let _eal = start_eal();` prologue and gains a #[with_eal]
attribute above #[test]. The inline start_eal helper and the
module-scoped OnceLock<Eal> static go away.
dpdk/Cargo.toml grows the `test` feature (turns on dpdk-test-macros
plus the id and nix runtime helpers that test_support needs) and a
self-referencing dev-dep `dataplane-dpdk = { path = ".", features =
["test"] }` so dpdk's own tests see the macro surface.
just fmt; cargo check --workspace --all-targets passes.
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Introduces a no_std FixedSize trait carrying a known byte width plus a big-endian write. Lives in its own crate so value-providing crates (`net`) and downstream key-packing frameworks (a future `match-action` PR) can both depend on it without depending on each other -- `net` implements FixedSize on its own newtypes without an orphan-rule violation, and the framework doesn't need to know about any particular value supplier. fixed-size/src/lib.rs: - pub trait FixedSize: Copy, with const SIZE: usize and fn write_be(&self, out: &mut [u8]). - Blanket impls for the network-order primitives the framework cares about: u8 / u16 / u32 / u64 / u128 / Ipv4Addr / Ipv6Addr. net/src/fixed_size.rs is the consumer bridge: FixedSize impls for TcpPort, UdpPort, UnicastIpv4Addr, and Vni. Each delegates write_be to the underlying primitive's impl, so wire bytes match what writing the raw integer would produce. Vni is a 24-bit value written into a 4-byte field (high byte zero) because backends model fields in 1/2/4 widths, not 3. net/Cargo.toml grows the fixed-size workspace dep; net/src/lib.rs adds the module behind no cfg gates (it has no further dependencies of its own). just fmt; cargo check --workspace --all-targets passes. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Adds the read-only key/action lookup vocabulary downstream match-action backends will implement. Tiny crate by design (one trait pair, two collection impls, inline tests) so consumer crates depend on this without depending on each other. lookup/src/lib.rs: - pub trait Projection<T>: extracts a key of type T from self. Implemented on `&'a Source` so the lifetime threads into T when T borrows; for owned T the lifetime is unused. The same source can implement Projection<T> for many T -- the call site picks one by inference from a Lookup backend's key type. - impl<K> Projection<Option<K>> for Option<K>: identity, so classify_opt accepts a pre-computed Option<K> directly. Scoped to Option<K> (not a general impl<T> Projection<T> for T) so a backend that implements Lookup<K, A> for every K can't make classify ambiguous. - pub trait Lookup<K, A>: lookup(&K) -> Option<&A>, plus default methods classify (S: Projection<K>) and classify_opt (S: Projection<Option<K>>) that project and look up in one call. Two trait type parameters, so one backend can serve many (K, A) pairs. - Blanket Lookup impls for BTreeMap<K, V> and HashMap<K, V, S> so tests / simple consumers get a working backend out of the box. Inline tests exercise: projection-by-table-type inference at v4 2-tuple and 4-tuple widths, lifetime threading through borrowed projections, miss returning None, classify_opt short-circuit on None projection, the identity Projection<Option<K>> for Option<K>, and HashMap parity with BTreeMap. just fmt; cargo check --workspace --all-targets passes. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Lands the match-action key vocabulary plus its derive. Backends in downstream PRs consume the result via a structural FIELD_SPECS view and a byte-packing as_key_into(); the bolero property-test generators land next as their own feature gate. match-action/src/: - lib.rs publishes the FieldKind enum (Prefix / Mask / Range / Exact), the FieldSpec layout record (name / kind / size / offset), and the MatchKey trait (N, KEY_SIZE, field_specs(), as_key_into()). Trait methods take slices rather than Self::N-sized arrays because Self::N in a trait fn needs unstable generic_const_exprs; the derive emits sized inherent helpers on concrete types instead. - field.rs is a thin re-export of FixedSize from the fixed-size crate, so consumers can refer to it via match_action::FixedSize. - predicate.rs holds the type-erased FieldPredicate form (Prefix, Mask, Range, Exact variants over FieldBytes) plus the Erased backend marker used by the reference oracle. - rule.rs defines the four kind-typed *Spec wrappers (PrefixSpec, MaskSpec, RangeSpec, ExactSpec), the Backend / Accepts / IntoBackendField / IsUniversal traits, and the RuleField envelope carrying name / spec. match-action-derive (proc-macro crate) emits, from a struct annotated with #[prefix] / #[mask] / #[range] / #[exact]: - the MatchKey impl, - a parallel <Name>Rule struct holding the typed *Spec data, - an inherent FIELD_SPECS const for compile-time inspection by downstream backends, and - (for non-generic structs) an inherent as_key() -> [u8; KEY_SIZE] for ergonomic byte packing. tests/derive_roundtrip.rs exercises a 5-tuple-shaped key covering all four predicate kinds: asserts N, KEY_SIZE, the field_specs() content, and that as_key_into / as_key produce the expected big-endian byte layout (with declared field ordering preserved). just fmt; cargo check --workspace --all-targets passes. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Adds property-test generators over the rule wrappers, behind a new `bolero` feature gate so the bolero dep stays out of the default build graph. Downstream consumers (acl property tests, future cascade Upsert-laws harnesses) enable the feature in their dev-deps and draw random matching / non-matching packets for any single rule. match-action/src/generator.rs: - FieldHit / FieldMiss TypeGenerators yielding bytes that satisfy / violate a given field predicate. Cover all four predicate kinds: Prefix (any address sharing the rule's high-order bits / one with flipped bits in that prefix), Mask (value bits match / a flipped in-mask bit), Range (uniform draw in [min, max] / outside the bounds), Exact (the value / a different value). - Miss generators skip universal predicates (range covering all values, mask of all zeros, prefix length zero, etc.); the derive-emitted IsUniversal check on <Name>Rule lets callers detect whole rules that have an empty miss set. match-action/src/lib.rs picks up #[cfg(feature = "bolero")] gates on the generator mod + its FieldHit / FieldMiss re-exports. match-action/Cargo.toml grows the bolero optional dep and the `bolero` feature, which also turns on the matching forwarded feature on match-action-derive (a no-op today, kept for future emit changes in the derive). match-action-derive/Cargo.toml mirrors the `bolero` feature declaration so cargo can forward through. just fmt; cargo check --workspace --all-targets and cargo check -p dataplane-match-action --features bolero pass. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
daniel-noland
force-pushed
the
pr/daniel-noland/dpdk-test-eal
branch
from
df2ad93 to
3741795
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
match-action/src/rule.rs (1)
73-77: 💤 Low valueConsider adding a debug assertion for
min <= max.If a caller passes
min > max, the resulting range will never match any value. Adding adebug_assert!would catch accidental inversions during development without affecting release performance.🛠️ Suggested defensive check
impl<T: FixedSize> RangeSpec<T> { #[must_use] pub const fn new(min: T, max: T) -> Self { + // Note: Can't use debug_assert! in const fn, but consider + // a non-const variant or runtime check if ordering matters. Self { min, max } }🤖 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 `@match-action/src/rule.rs` around lines 73 - 77, Add a debug assertion in RangeSpec::new to catch inverted bounds: when constructing RangeSpec<T> (impl<T: FixedSize> RangeSpec<T>::new), insert a debug_assert!(min <= max) so accidental calls with min > max are detected in debug builds without affecting release performance.
🤖 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.
Inline comments:
In `@match-action/src/generator.rs`:
- Around line 265-300: PredicateHitsBytes::generate assumes range bounds fit in
4 bytes via be_to_u32/u32_to_be; add an explicit guard in the range branch
(inside PredicateHitsBytes::generate where you call self.pred.as_range()) to
check min.len() and max.len() (or just min.len()) <= 4 and return None if they
are larger, or alternatively handle wider sizes via a larger integer conversion
path; update predicate_hits_bytes/PredicateHitsBytes generate to perform this
length check before calling be_to_u32/u32_to_be so the method fails safely
instead of panicking.
---
Nitpick comments:
In `@match-action/src/rule.rs`:
- Around line 73-77: Add a debug assertion in RangeSpec::new to catch inverted
bounds: when constructing RangeSpec<T> (impl<T: FixedSize> RangeSpec<T>::new),
insert a debug_assert!(min <= max) so accidental calls with min > max are
detected in debug builds without affecting release performance.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c038aac-439e-433e-89f9-2d45b1181310
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomldpdk-test-macros/Cargo.tomldpdk-test-macros/src/lib.rsdpdk/Cargo.tomldpdk/src/acl/config.rsdpdk/src/acl/mod.rsdpdk/src/lib.rsdpdk/src/test_support.rsfixed-size/Cargo.tomlfixed-size/src/lib.rslookup/Cargo.tomllookup/src/lib.rsmatch-action-derive/Cargo.tomlmatch-action-derive/src/lib.rsmatch-action/Cargo.tomlmatch-action/src/field.rsmatch-action/src/generator.rsmatch-action/src/lib.rsmatch-action/src/predicate.rsmatch-action/src/rule.rsmatch-action/tests/derive_roundtrip.rsnet/Cargo.tomlnet/src/fixed_size.rsnet/src/lib.rs
✅ Files skipped from review due to trivial changes (4)
- match-action-derive/Cargo.toml
- match-action/Cargo.toml
- match-action/tests/derive_roundtrip.rs
- net/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- net/src/lib.rs
- dpdk/src/lib.rs
- fixed-size/Cargo.toml
- dpdk-test-macros/Cargo.toml
- lookup/Cargo.toml
- fixed-size/src/lib.rs
- dpdk-test-macros/src/lib.rs
- dpdk/Cargo.toml
- net/src/fixed_size.rs
- dpdk/src/test_support.rs
- dpdk/src/acl/config.rs
- lookup/src/lib.rs
- dpdk/src/acl/mod.rs
Comment on lines
+265
to
+300
| #[must_use] | ||
| pub fn predicate_hits_bytes(pred: FieldPredicate) -> PredicateHitsBytes { | ||
| PredicateHitsBytes { pred } | ||
| } | ||
| #[must_use] | ||
| pub fn predicate_misses_bytes(pred: FieldPredicate) -> PredicateMissesBytes { | ||
| let universal = predicate_is_universal(&pred); | ||
| PredicateMissesBytes { pred, universal } | ||
| } | ||
|
|
||
| pub struct PredicateHitsBytes { | ||
| pred: FieldPredicate, | ||
| } | ||
|
|
||
| impl ValueGenerator for PredicateHitsBytes { | ||
| type Output = Vec<u8>; | ||
| fn generate<D: Driver>(&self, d: &mut D) -> Option<Vec<u8>> { | ||
| if let Some(value) = self.pred.as_exact() { | ||
| Some(value.to_vec()) | ||
| } else if let Some((value, prefix_len)) = self.pred.as_prefix() { | ||
| let mut buf = draw_bytes(d, value.len())?; | ||
| splat_prefix(&mut buf, value, prefix_len); | ||
| Some(buf) | ||
| } else if let Some((value, mask)) = self.pred.as_mask() { | ||
| let mut buf = draw_bytes(d, value.len())?; | ||
| splat_under_mask(&mut buf, value, mask); | ||
| Some(buf) | ||
| } else if let Some((min, max)) = self.pred.as_range() { | ||
| let lo = be_to_u32(min); | ||
| let hi = be_to_u32(max); | ||
| let v = (lo..=hi).generate(d)?; | ||
| Some(u32_to_be(v, min.len())) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if RangeSpec is ever used with types larger than 4 bytes
rg -n 'RangeSpec<.*(u64|u128|Ipv6)' --type rustRepository: githedgehog/dataplane
Length of output: 47
Document/guard the 4-byte assumption for range predicate byte generation.
PredicateHitsBytes converts range bounds via be_to_u32/u32_to_be, so FieldPredicate::Range must not carry >4-byte widths (or generate() will blow up instead of returning None). A repo search for RangeSpec<...u64|u128|Ipv6> found no matches, suggesting wider ranges aren’t currently used, but it’s still worth documenting the constraint or adding an explicit guard.
🤖 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 `@match-action/src/generator.rs` around lines 265 - 300,
PredicateHitsBytes::generate assumes range bounds fit in 4 bytes via
be_to_u32/u32_to_be; add an explicit guard in the range branch (inside
PredicateHitsBytes::generate where you call self.pred.as_range()) to check
min.len() and max.len() (or just min.len()) <= 4 and return None if they are
larger, or alternatively handle wider sizes via a larger integer conversion
path; update predicate_hits_bytes/PredicateHitsBytes generate to perform this
length check before calling be_to_u32/u32_to_be so the method fails safely
instead of panicking.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@fixed-size/src/lib.rs`:
- Around line 15-18: Document that FixedSize::write_be requires out.len() >=
Self::SIZE and update the trait doc comment to state this precondition; then
either add debug checks in implementations (insert debug_assert!(out.len() >=
Self::SIZE) at the top of each write_be impl) or change the trait signature (fn
write_be(&self, out: &mut [u8]) -> Result<(), BufferTooShortError>) and update
all implementations and callers accordingly so buffer-short conditions are
handled without violating #![deny(clippy::panic)].
- Around line 20-53: The write_be implementations for the FixedSize trait (for
u8, u16, u32, u64, u128 and the Ipv4Addr/Ipv6Addr impls) lack bounds checks and
will panic when out.len() < Self::SIZE; add a defensive check at the start of
each impl's write_be method such as a debug_assert!(out.len() >= Self::SIZE) (or
equivalent non-panicking guard) to validate the buffer length before writing,
ensuring the implementations of FixedSize::write_be perform the length
verification.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: abe2ddf8-84e7-4559-8da9-f75fdbd4a789
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.tomldpdk-test-macros/Cargo.tomldpdk-test-macros/src/lib.rsdpdk/Cargo.tomldpdk/src/acl/config.rsdpdk/src/acl/mod.rsdpdk/src/lib.rsdpdk/src/test_support.rsfixed-size/Cargo.tomlfixed-size/src/lib.rslookup/Cargo.tomllookup/src/lib.rsmatch-action-derive/Cargo.tomlmatch-action-derive/src/lib.rsmatch-action/Cargo.tomlmatch-action/src/field.rsmatch-action/src/generator.rsmatch-action/src/lib.rsmatch-action/src/predicate.rsmatch-action/src/rule.rsmatch-action/tests/derive_roundtrip.rsnet/Cargo.tomlnet/src/fixed_size.rsnet/src/lib.rs
✅ Files skipped from review due to trivial changes (4)
- lookup/Cargo.toml
- fixed-size/Cargo.toml
- net/Cargo.toml
- match-action-derive/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (18)
- match-action/Cargo.toml
- dpdk/src/lib.rs
- dpdk-test-macros/Cargo.toml
- net/src/fixed_size.rs
- lookup/src/lib.rs
- match-action/src/field.rs
- net/src/lib.rs
- dpdk/src/acl/config.rs
- dpdk/Cargo.toml
- dpdk-test-macros/src/lib.rs
- Cargo.toml
- match-action/tests/derive_roundtrip.rs
- dpdk/src/test_support.rs
- match-action/src/lib.rs
- match-action/src/predicate.rs
- match-action/src/generator.rs
- match-action-derive/src/lib.rs
- match-action/src/rule.rs
Comment on lines
+15
to
+18
| pub trait FixedSize: Copy { | ||
| const SIZE: usize; | ||
| fn write_be(&self, out: &mut [u8]); | ||
| } |
There was a problem hiding this comment.
Document the buffer size precondition and consider adding safety checks.
The trait lacks documentation explaining that write_be requires out.len() >= Self::SIZE. All implementations will panic if this precondition is violated, which contradicts the #![deny(clippy::panic)] lint on line 11.
Consider:
- Adding comprehensive trait documentation explaining the
write_beprecondition - Adding
debug_assert!(out.len() >= Self::SIZE)in each implementation (panics only in debug builds) - Or changing the signature to return
Result<(), BufferTooShortError>for graceful handling
📋 Proposed trait documentation
+/// A trait for types that can be serialized to a fixed-size big-endian byte representation.
+///
+/// # Contract
+///
+/// - `SIZE` specifies the exact number of bytes required for serialization
+/// - `write_be` writes exactly `SIZE` bytes in big-endian (network) byte order
+/// - The caller **must** ensure `out.len() >= Self::SIZE` before calling `write_be`
+///
+/// # Panics
+///
+/// Implementations will panic if `out.len() < Self::SIZE`.
pub trait FixedSize: Copy {
const SIZE: usize;
+ /// Serialize `self` into the first `Self::SIZE` bytes of `out` in big-endian order.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `out.len() < Self::SIZE`.
fn write_be(&self, out: &mut [u8]);
}🤖 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 `@fixed-size/src/lib.rs` around lines 15 - 18, Document that
FixedSize::write_be requires out.len() >= Self::SIZE and update the trait doc
comment to state this precondition; then either add debug checks in
implementations (insert debug_assert!(out.len() >= Self::SIZE) at the top of
each write_be impl) or change the trait signature (fn write_be(&self, out: &mut
[u8]) -> Result<(), BufferTooShortError>) and update all implementations and
callers accordingly so buffer-short conditions are handled without violating
#![deny(clippy::panic)].
Comment on lines
+20
to
+53
| impl FixedSize for u8 { | ||
| const SIZE: usize = 1; | ||
| fn write_be(&self, out: &mut [u8]) { | ||
| out[0] = *self; | ||
| } | ||
| } | ||
|
|
||
| impl FixedSize for u16 { | ||
| const SIZE: usize = 2; | ||
| fn write_be(&self, out: &mut [u8]) { | ||
| out[..Self::SIZE].copy_from_slice(&self.to_be_bytes()); | ||
| } | ||
| } | ||
|
|
||
| impl FixedSize for u32 { | ||
| const SIZE: usize = 4; | ||
| fn write_be(&self, out: &mut [u8]) { | ||
| out[..Self::SIZE].copy_from_slice(&self.to_be_bytes()); | ||
| } | ||
| } | ||
|
|
||
| impl FixedSize for u64 { | ||
| const SIZE: usize = 8; | ||
| fn write_be(&self, out: &mut [u8]) { | ||
| out[..Self::SIZE].copy_from_slice(&self.to_be_bytes()); | ||
| } | ||
| } | ||
|
|
||
| impl FixedSize for u128 { | ||
| const SIZE: usize = 16; | ||
| fn write_be(&self, out: &mut [u8]) { | ||
| out[..Self::SIZE].copy_from_slice(&self.to_be_bytes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add defensive bounds checking to prevent panics on invalid input.
Each implementation will panic if out.len() < Self::SIZE, but there's no validation. This violates the #![deny(clippy::panic)] directive at line 11.
🛡️ Proposed fix with debug assertions
Example pattern for all implementations:
impl FixedSize for u8 {
const SIZE: usize = 1;
fn write_be(&self, out: &mut [u8]) {
+ debug_assert!(
+ out.len() >= Self::SIZE,
+ "write_be: buffer too short (need {}, got {})",
+ Self::SIZE,
+ out.len()
+ );
out[0] = *self;
}
}
impl FixedSize for u16 {
const SIZE: usize = 2;
fn write_be(&self, out: &mut [u8]) {
+ debug_assert!(
+ out.len() >= Self::SIZE,
+ "write_be: buffer too short (need {}, got {})",
+ Self::SIZE,
+ out.len()
+ );
out[..Self::SIZE].copy_from_slice(&self.to_be_bytes());
}
}Apply the same pattern to u32, u64, u128, Ipv4Addr, and Ipv6Addr.
🤖 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 `@fixed-size/src/lib.rs` around lines 20 - 53, The write_be implementations for
the FixedSize trait (for u8, u16, u32, u64, u128 and the Ipv4Addr/Ipv6Addr
impls) lack bounds checks and will panic when out.len() < Self::SIZE; add a
defensive check at the start of each impl's write_be method such as a
debug_assert!(out.len() >= Self::SIZE) (or equivalent non-panicking guard) to
validate the buffer length before writing, ensuring the implementations of
FixedSize::write_be perform the length verification.
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
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.
Stack (8). Base:
pr/daniel-noland/threading-rewrite.DPDK test plumbing that the rest of the ACL stack relies on, kept separate so it
reviews on its own:
refactor(dpdk/acl): makecompute_min_input_sizeaconst fn.feat(dpdk):test_support::start_eal+#[with_eal]attribute macro(
dpdk-test-macros), so EAL-dependent tests can opt in declaratively.No new abstractions, no behavior change to shipping code.
Review stack (merge bottom -> top):
Summary
This PR adds DPDK test harnessing and supporting workspace crates, plus a refactor to make an ACL helper const-evaluable. Key changes:
refactor(dpdk/acl): make compute_min_input_size const
feat(dpdk): EAL test harness and #[with_eal] macro
testfeature; dpdk tests were updated to use #[with_eal] (removing inline per-test start_eal and module-scoped OnceLock).workspace & crate additions / Cargo wiring
testfeature that wires optional deps (id, nix with sched feature, dpdk-test-macros) and replaced prior dev-only id/nix usage with a self-referencing dev-dependency using the newtestfeature so test-only helpers/macros are available to dpdk tests.New crates and notable APIs
Tests and usage
Scope & compatibility
Review notes