feat(test): BPF_PROG_TEST_RUN for unittest. #1461
feat(test): BPF_PROG_TEST_RUN for unittest. #1461hsqStephenZhang wants to merge 4 commits intoaya-rs:mainfrom
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@dave-tucker would you like to take a look? |
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 6 files and all commit messages, and made 7 comments.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @hsqStephenZhang).
aya/src/sys/bpf.rs line 587 at r2 (raw file):
pub cpu: u32, /// Batch size for network packet tests (requires BPF_F_TEST_XDP_LIVE_FRAMES flag). pub batch_size: u32,
if these flags are required, maybe this would be better expressed as an enum? or else, it would better expressed as an option, and the implementation would add the flags?
Code quote:
/// CPU to run the test on (requires BPF_F_TEST_RUN_ON_CPU flag).
pub cpu: u32,
/// Batch size for network packet tests (requires BPF_F_TEST_XDP_LIVE_FRAMES flag).
pub batch_size: u32,aya/src/sys/bpf.rs line 614 at r2 (raw file):
let test = unsafe { &mut attr.test }; test.prog_fd = prog_fd.as_raw_fd() as u32; // at least 1
why? if you want the default to be 1, that's fine, but that shouldn't be here.
test/integration-ebpf/src/test_run.rs line 27 at r2 (raw file):
unsafe fn try_test_xdp(_ctx: XdpContext) -> Result<u32, u32> { Ok(xdp_action::XDP_PASS) }
why unsafe? also, just inline it
Code quote:
unsafe fn try_test_xdp(_ctx: XdpContext) -> Result<u32, u32> {
Ok(xdp_action::XDP_PASS)
}test/integration-ebpf/src/test_run.rs line 72 at r2 (raw file):
} Ok(xdp_action::XDP_PASS)
inline this please, this never returns an error.
Code quote:
unsafe fn try_test_xdp_modify(ctx: XdpContext) -> Result<u32, u32> {
let data = ctx.data();
let data_end = ctx.data_end();
if data + 16 > data_end {
return Ok(xdp_action::XDP_PASS);
}
let packet = data as *mut u8;
for i in 0..16 {
unsafe {
*packet.add(i) = 0xAAu8;
}
}
Ok(xdp_action::XDP_PASS)test/integration-ebpf/src/test_run.rs line 95 at r2 (raw file):
Ok(xdp_action::XDP_DROP) } }
inline.
Code quote:
unsafe fn try_test_xdp_context(ctx: XdpContext) -> Result<u32, u32> {
// hardcoded expected value
const EXPECTED_IF: u32 = 1;
let md = ctx.ctx;
let rx_queue = unsafe { (*md).ingress_ifindex };
if rx_queue == EXPECTED_IF {
Ok(xdp_action::XDP_PASS)
} else {
Ok(xdp_action::XDP_DROP)
}
}aya/src/programs/mod.rs line 879 at r2 (raw file):
); macro_rules! impl_program_test_run {
this macro is a bit of an odd choice. do we have an existing macro used for all programs or a trait we could add this to? even if the answer is no on both counts, I think an extension trait would be nicer than a macro (we can still use a macro for the empty impls)
aya/src/sys/mod.rs line 19 at r2 (raw file):
use aya_obj::generated::{bpf_attr, bpf_cmd, perf_event_attr}; pub(crate) use bpf::*; pub use bpf::{TestRunOptions, TestRunResult};
can this pub export be at the top level of the crate? i don't think we want the API to be sys::TestRunXXX
|
thanks for the suggestions! |
There was a problem hiding this comment.
Pull request overview
This PR implements the BPF_PROG_TEST_RUN syscall functionality for unit testing BPF programs, addressing issue #36. The implementation provides a convenient API for testing BPF programs without needing to attach them to live system events.
Changes:
- Adds
TestRunOptionsandTestRunResulttypes to configure and capture test execution results - Implements
TestRuntrait for supported program types (SocketFilter, SchedClassifier, Xdp, CgroupSkb, SkMsg, SkSkb, SockOps, FlowDissector, RawTracePoint) - Adds comprehensive integration tests demonstrating test_run usage with various program types and options
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/public-api/aya.txt | Tracks new public API surface including TestRunOptions, TestRunResult, and test_run methods |
| aya/src/sys/bpf.rs | Core implementation of test_run syscall wrapper with TestRunOptions and TestRunResult types |
| aya/src/sys/mod.rs | Re-exports TestRunOptions and TestRunResult for public API |
| aya/src/programs/mod.rs | Defines TestRun trait and implements it for supported program types |
| aya/src/lib.rs | Re-exports TestRunOptions and TestRunResult at crate root |
| test/integration-test/src/tests/prog_test_run.rs | Integration tests covering basic test_run, packet modification, context handling, and repeat execution |
| test/integration-test/src/tests.rs | Registers new prog_test_run test module |
| test/integration-test/src/lib.rs | Adds TEST_RUN BPF program to test fixtures |
| test/integration-ebpf/src/test_run.rs | BPF test programs for integration testing |
| test/integration-ebpf/Cargo.toml | Adds test_run binary configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// Sets the CPU to run the test on. | ||
| /// This automatically sets the `BPF_F_TEST_RUN_ON_CPU` flag. | ||
| /// This option only works with `TracePoint` programs. |
There was a problem hiding this comment.
Documentation mentions "TracePoint programs" but according to the PR description and the implementation (line 950), only RawTracePoint supports test_run, not TracePoint. The comment should be updated to say "RawTracePoint programs" or "raw tracepoint programs" to avoid confusion.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
78ccdeb to
5fceff7
Compare
tamird
left a comment
There was a problem hiding this comment.
@codex review
@tamird reviewed 9 files and all commit messages, made 13 comments, and resolved 6 discussions.
Reviewable status: 9 of 10 files reviewed, 13 unresolved discussions (waiting on @hsqStephenZhang).
aya/src/sys/mod.rs line 19 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can this pub export be at the top level of the crate? i don't think we want the API to be sys::TestRunXXX
clarifying: it should not be pub here.
test/integration-ebpf/src/test_run.rs line 63 at r22 (raw file):
#[xdp] fn test_xdp_context(ctx: XdpContext) -> u32 { // hardcoded expected value
useless comment?
test/integration-test/src/tests/prog_test_run.rs line 21 at r22 (raw file):
#[test_log::test] fn test_xdp_test_run() { if !require_version(5, 18, 0) {
could you please add citations for these version numbers?
test/integration-test/src/tests/prog_test_run.rs line 32 at r22 (raw file):
let mut data_out = vec![0u8; 64]; let mut opts = TestRunOptions::default();
let opts = TestRunOptions {
data_in: Some(&mut data_in),
data_out: Some(&mut data out),
...Default::default()
}
test/integration-test/src/tests/prog_test_run.rs line 36 at r22 (raw file):
opts.data_out = Some(&mut data_out); let result = prog.test_run(&mut opts).unwrap();
destructure please
aya/src/sys/bpf.rs line 571 at r22 (raw file):
} /// Options for running a BPF program test.
blank doc comment line between paragraphs, please.
aya/src/sys/bpf.rs line 586 at r22 (raw file):
pub repeat: u32, // CPU to run the test on, only works with RawTracePoint programs. // should be set via `run_on_cpu` function to set the appropriate flag.
can you just remove this? unclear who it's for, and it's duplicating the comment on run_on_cpu
Code quote:
// CPU to run the test on, only works with RawTracePoint programs.
// should be set via `run_on_cpu` function to set the appropriate flag.aya/src/sys/bpf.rs line 589 at r22 (raw file):
cpu: u32, // Batch size for network packet tests, only works with XDP programs. // should be set via `xdp_live_frames` function to set the appropriate flag.
ditto, just remove
Code quote:
// Batch size for network packet tests, only works with XDP programs.
// should be set via `xdp_live_frames` function to set the appropriate flag.aya/src/sys/bpf.rs line 591 at r22 (raw file):
// should be set via `xdp_live_frames` function to set the appropriate flag. batch_size: u32, // Flags to control test behavior.
remove
aya/src/sys/bpf.rs line 612 at r22 (raw file):
impl TestRunOptions<'_> { /// Creates a new `TestRunOptions` with default values. pub fn new() -> Self {
move the impl from Default to here and make this function const please.
aya/src/sys/bpf.rs line 618 at r22 (raw file):
/// Sets the CPU to run the test on. /// This automatically sets the `BPF_F_TEST_RUN_ON_CPU` flag. /// This option only works with `TracePoint` programs.
wrong?
aya/src/sys/bpf.rs line 655 at r22 (raw file):
pub(crate) fn bpf_prog_test_run( prog_fd: BorrowedFd<'_>, opts: &TestRunOptions<'_>,
could you please destructure opts at the top of the function so it's easy to see (and the compiler can check) that all fields are used?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 297e8a05e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e3ac957 to
8f81115
Compare
|
most comments are fixed, except:
|
tamird
left a comment
There was a problem hiding this comment.
for
TestRunOptions, should i move it and place it together withTestRuntrait to keep the visibility cleaner?
Um, sure.
@tamird reviewed 3 files and all commit messages, made 6 comments, and resolved 3 discussions.
Reviewable status: 9 of 10 files reviewed, 11 unresolved discussions (waiting on @hsqStephenZhang).
aya/src/sys/bpf.rs line 586 at r22 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can you just remove this? unclear who it's for, and it's duplicating the comment on
run_on_cpu
Just remove the whole comment?
aya/src/sys/bpf.rs line 589 at r22 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto, just remove
Remove the whole comment?
aya/src/sys/bpf.rs line 591 at r22 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
remove
?
aya/src/sys/bpf.rs line 655 at r22 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
could you please destructure
optsat the top of the function so it's easy to see (and the compiler can check) that all fields are used?
?
aya/src/sys/bpf.rs line 596 at r24 (raw file):
impl TestRunOptions<'_> { /// Creates a new `TestRunOptions` with default values. #[expect(clippy::new_without_default, reason = "ignore default method")]
You can still implement Default by calling new.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 5 files and all commit messages, made 8 comments, and resolved 9 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @hsqStephenZhang).
test/integration-test/src/tests/prog_test_run.rs line 21 at r22 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
could you please add citations for these version numbers?
this still needs doing
test/integration-test/src/tests/prog_test_run.rs line 36 at r22 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
destructure please
?
test/integration-test/src/tests/prog_test_run.rs line 30 at r26 (raw file):
let data_in = vec![0u8; 64]; let mut data_out = vec![0u8; 64];
magic numbers
Code quote:
let data_in = vec![0u8; 64];
let mut data_out = vec![0u8; 64];test/integration-test/src/tests/prog_test_run.rs line 57 at r26 (raw file):
let data_in = vec![0u8; 64]; let mut data_out = vec![0u8; 64];
can we avoid these magic numbers?
Code quote:
let data_in = vec![0u8; 64];
let mut data_out = vec![0u8; 64];test/integration-test/src/tests/prog_test_run.rs line 70 at r26 (raw file):
let expected_pattern: Vec<u8> = vec![0xAAu8; 16]; assert_eq!(&data_out[..16], &*expected_pattern,); assert_eq!(&data_out[16..], &data_in[16..],);
errant trailing commas
Code quote:
assert_eq!(&data_out[..16], &*expected_pattern,);
assert_eq!(&data_out[16..], &data_in[16..],);test/integration-test/src/tests/prog_test_run.rs line 204 at r26 (raw file):
// for interface 1. // 6. The egress cannot be specified // see: https://github.com/torvalds/linux/blob/63804fed149a6750ffd28610c5c1c98cce6bd377/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c#L92
let's save some screen width and use https://github.com/torvalds/linux/blob/63804fed/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c#L92
test/integration-test/src/tests/prog_test_run.rs line 218 at r26 (raw file):
let size = size_of::<XdpMd>(); let ctx_bytes = unsafe { std::slice::from_raw_parts((&raw const ctx).cast::<u8>(), size) };
this looks a lot like fn bytes_of which we have a few copies of. it would be OK to have another copy here
test/integration-ebpf/src/test_run.rs line 54 at r26 (raw file):
for i in 0..16 { unsafe { *packet.add(i) = 0xAAu8;
this magic value is shared between ebpf and userspace. can you put it in the common crate?
ditto for the magic number 16
3d76a97 to
db39b89
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 3 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hsqStephenZhang).
test/integration-common/src/lib.rs line 117 at r27 (raw file):
pub mod test_run { pub const XDP_MODIGY_VAL: u8 = 0xAA;
MODIFY?
test/integration-test/src/tests/prog_test_run.rs line 9 at r28 (raw file):
use integration_common::test_run::{IF_INDEX, XDP_MODIGY_LEN, XDP_MODIGY_VAL}; // https://github.com/torvalds/linux/blob/8fdb05de0e2db89d8f56144c60ab784812e8c3b7/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c#L48
(shorter, don't need the full hash)
also this link does not explain how you got the v4 packet size
b2042d0 to
cebd6c4
Compare
|
@hsqStephenZhang could you please reply to all the comments in reviewable? |
cebd6c4 to
eb0484c
Compare
hsqStephenZhang
left a comment
There was a problem hiding this comment.
fixed.
@hsqStephenZhang made 6 comments and resolved 3 discussions.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on tamird).
| data_out: None, | ||
| ctx_in: None, | ||
| ctx_out: None, | ||
| repeat: 1, |
There was a problem hiding this comment.
defaults to 1, with more documents on how to use it correctly.
| /// let mut opts = TestRunOptions { | ||
| /// data_in: Some(&input_data), | ||
| /// data_out: Some(&mut output_data), | ||
| /// repeat: 1, | ||
| /// ..Default::default() | ||
| /// }; | ||
| /// | ||
| /// let result = program.test_run(&mut opts)?; | ||
| /// println!("Program returned: {}, took {} ns", result.return_value, result.duration); |
| SkMsg, | ||
| SkSkb, | ||
| SockOps, |
| if let Some(data_out) = data_out { | ||
| test.data_out = data_out.as_ptr() as u64; | ||
| test.data_size_out = data_out.len() as u32; | ||
| } | ||
|
|
||
| if let Some(ctx_in) = ctx_in { | ||
| test.ctx_in = ctx_in.as_ptr() as u64; | ||
| test.ctx_size_in = ctx_in.len() as u32; | ||
| } | ||
|
|
||
| if let Some(ctx_out) = ctx_out { | ||
| test.ctx_out = ctx_out.as_ptr() as u64; | ||
| test.ctx_size_out = ctx_out.len() as u32; | ||
| } |
| fn bytes_of<T: Sized>(val: &T) -> &[u8] { | ||
| let size = size_of::<T>(); | ||
| unsafe { core::slice::from_raw_parts(core::ptr::from_ref::<T>(val).cast::<u8>(), size) } | ||
| } |
There was a problem hiding this comment.
not the case, it compiles. done.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 959 at r65 (raw file):
/// Must be `None` for [`RawTracePoint`] programs; the kernel returns /// `EINVAL` otherwise. pub data_out: Option<&'a mut [u8]>,
can we have a safer solution for this? ideally we would prevent misuse at compile time.
same for repeat below
Code quote:
/// Input data to pass to the program.
///
/// Must be `None` for [`RawTracePoint`] programs; the kernel returns
/// `EINVAL` otherwise.
pub data_in: Option<&'a [u8]>,
/// Output buffer for data modified by the program.
///
/// Must be `None` for [`RawTracePoint`] programs; the kernel returns
/// `EINVAL` otherwise.
pub data_out: Option<&'a mut [u8]>,
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on tamird).
aya/src/programs/mod.rs line 959 at r65 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we have a safer solution for this? ideally we would prevent misuse at compile time.
same for
repeatbelow
i could add a new RawTracePointRunOptions and remove the unusable fields.
tamird
left a comment
There was a problem hiding this comment.
@tamird made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 959 at r65 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
i could add a new RawTracePointRunOptions and remove the unusable fields.
Sure, and I guess TestRunOptions would include RawTracePointRunOptions + additional fields?
You'll have to be a bit careful about you pass the right type, though.
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on tamird).
aya/src/programs/mod.rs line 959 at r65 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Sure, and I guess TestRunOptions would include RawTracePointRunOptions + additional fields?
You'll have to be a bit careful about you pass the right type, though.
actually i was thinking about making them totally separate options and a associate type for trait TestRun, otherwise it's hard to prevent the user from modifying the fields that they should not.
tamird
left a comment
There was a problem hiding this comment.
@tamird made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 959 at r65 (raw file):
otherwise it's hard to prevent the user from modifying the fields that they should not.
i don't get it, if i'm holding a RawTracePointRunOptions then I can't modify things not in it. anyway an associated type makes sense i think
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 1 comment.
Reviewable status: 5 of 11 files reviewed, 1 unresolved discussion (waiting on tamird).
aya/src/programs/mod.rs line 959 at r65 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
otherwise it's hard to prevent the user from modifying the fields that they should not.
i don't get it, if i'm holding a
RawTracePointRunOptionsthen I can't modify things not in it. anyway an associated type makes sense i think
updated, also added new test case for RawTracePoint program.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 6 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 1008 at r67 (raw file):
#[derive(Debug)] pub struct RawTracePointRunOptions<'a> { /// Fake tracepoint arguments as a packed `[u64]` byte slice.
u64 or u8? if the former, this means the user can pass something malformed here...?
aya/src/programs/mod.rs line 1010 at r67 (raw file):
/// Fake tracepoint arguments as a packed `[u64]` byte slice. /// /// The kernel copies these bytes into the BPF register file (r1–r5) before
what if i pass more than 5*8 = 40 bytes? IOW should we require this to be a [u64;5] instead?
aya/src/sys/bpf.rs line 705 at r67 (raw file):
Ok(TestRunResult { return_value: test.retval, // bpf_prog_test_run_raw_tp does not fill in duration; it is always 0.
that's surprising, citation?
aya/src/sys/bpf.rs line 708 at r67 (raw file):
duration: std::time::Duration::ZERO, data_size_out: 0, ctx_size_out: 0,
if all these are always zero then we should have a separate type
Code quote:
duration: std::time::Duration::ZERO,
data_size_out: 0,
ctx_size_out: 0,
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 4 comments.
Reviewable status: 8 of 11 files reviewed, 4 unresolved discussions (waiting on tamird).
aya/src/programs/mod.rs line 1008 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
u64 or u8? if the former, this means the user can pass something malformed here...?
updated.
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
what if i pass more than 5*8 = 40 bytes? IOW should we require this to be a [u64;5] instead?
https://github.com/torvalds/linux/blob/d91a46d6805af41e7f2286e0fc22d498f45a682b/net/bpf/test_run.c#L761
there are two constraints, one for verifier, another for the MAX NUM, i prefer &[u64], what do you say? i've added some documents about that for RawTracePointRunOptions, check it out.
aya/src/sys/bpf.rs line 705 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
that's surprising, citation?
done.
aya/src/sys/bpf.rs line 708 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
if all these are always zero then we should have a separate type
it could make the API clumsy in my view. I prefer to leave the struct unchanged, only with more detailed documents. What do you say?
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 3 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
https://github.com/torvalds/linux/blob/d91a46d6805af41e7f2286e0fc22d498f45a682b/net/bpf/test_run.c#L761there are two constraints, one for verifier, another for the MAX NUM, i prefer
&[u64], what do you say? i've added some documents about that forRawTracePointRunOptions, check it out.
Thanks for adding the citation. But I still don't understand why you don't want to make it an array of MAX_BPF_FUNC_ARGS elements. The default would make it all-zero, so it's just as ergonomic with better type safety.
aya/src/sys/bpf.rs line 708 at r67 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
it could make the API clumsy in my view. I prefer to leave the struct unchanged, only with more detailed documents. What do you say?
Why is it clumsy? It's reducing noise in the API.
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 2 comments.
Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions (waiting on tamird).
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Thanks for adding the citation. But I still don't understand why you don't want to make it an array of
MAX_BPF_FUNC_ARGSelements. The default would make it all-zero, so it's just as ergonomic with better type safety.
fixed.
aya/src/sys/bpf.rs line 708 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Why is it clumsy? It's reducing noise in the API.
fixed.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 5 files and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
fixed.
why 12 rather than MAX_BPF_FUNC_ARGS? is that constant not available to us?
aya/src/programs/mod.rs line 1034 at r69 (raw file):
} /// Sets the tracepoint argument at `idx` to `value`.
can we just make args pub or expose a &mut [u64; 12] getter? we're erasing type information here and laying a panic trap for the caller
test/integration-test/src/tests/prog_test_run.rs line 284 at r69 (raw file):
assert_eq!(return_value, 0); assert_eq!(data_size_out, 0); assert_eq!(ctx_size_out, 0);
do we ever test a case where this is non-zero?
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on tamird).
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why
12rather thanMAX_BPF_FUNC_ARGS? is that constant not available to us?
it's just a macro, not available.
aya/src/programs/mod.rs line 1034 at r69 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we just make
argspub or expose a &mut [u64; 12] getter? we're erasing type information here and laying a panic trap for the caller
the args is already pub, i add this method to allow user write chain style constructionRawTracePointRunOptions::default().arg(0, SENTINEL);
do you prefer removing this method?
test/integration-test/src/tests/prog_test_run.rs line 284 at r69 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
do we ever test a case where this is non-zero?
there is a assert_eq!(ctx_size_out as usize, size_of::<XdpMd>());
tamird
left a comment
There was a problem hiding this comment.
@tamird made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on hsqStephenZhang).
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
it's just a macro, not available.
ah I think we can add this to our codegen
I can do it in a followup.
aya/src/programs/mod.rs line 1034 at r69 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
the
argsis already pub, i add this method to allow user write chain style constructionRawTracePointRunOptions::default().arg(0, SENTINEL);do you prefer removing this method?
Yes please
test/integration-test/src/tests/prog_test_run.rs line 284 at r69 (raw file):
Previously, hsqStephenZhang (z combinator) wrote…
there is a
assert_eq!(ctx_size_out as usize, size_of::<XdpMd>());
ah thanks
hsqStephenZhang
left a comment
There was a problem hiding this comment.
@hsqStephenZhang made 3 comments.
Reviewable status: 8 of 11 files reviewed, 2 unresolved discussions (waiting on tamird).
aya/src/programs/mod.rs line 1010 at r67 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ah I think we can add this to our codegen
I can do it in a followup.
thanks.
aya/src/programs/mod.rs line 1034 at r69 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Yes please
done.
test/integration-test/src/tests/prog_test_run.rs line 284 at r69 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ah thanks
sure
|
Merge conflicts. |
77fe695 to
98638f4
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 8 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on hsqStephenZhang).
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// # Returns | ||
| /// | ||
| /// Returns a [`crate::TestRunResult`] containing: | ||
| /// - `return_value`: The value returned by the program | ||
| /// - `duration`: Execution time in nanoseconds | ||
| /// - `data_size_out`: Size of data written to output buffer | ||
| /// - `ctx_size_out`: Size of context written to output buffer | ||
| /// |
There was a problem hiding this comment.
The TestRun trait is implemented for multiple program types with different associated Result types (e.g. RawTracePoint returns RawTracePointTestRunResult without duration), but these docs currently hard-code the return type as crate::TestRunResult and describe fields that aren’t present for all implementors. Please update the docs to describe Self::Result (and, if needed, document per-program-type differences).
| /// Sets the CPU to run the test on. | ||
| /// | ||
| /// This automatically sets the `BPF_F_TEST_RUN_ON_CPU` flag. | ||
| /// This option only works with `RawTracePoint` programs. | ||
| #[must_use] | ||
| pub const fn run_on_cpu(mut self, cpu: u32) -> Self { | ||
| self.cpu = cpu; | ||
| self.flags |= BPF_F_TEST_RUN_ON_CPU; | ||
| self |
There was a problem hiding this comment.
The docs for TestRunAttrs::run_on_cpu say it “only works with RawTracePoint programs”, but RawTracePoint doesn’t use TestRunOptions/TestRunAttrs at all (it uses RawTracePointRunOptions). This is misleading; please correct the doc to reflect the actual API surface (either describe the supported program types for TestRunOptions, or remove the RawTracePoint-specific claim).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98638f40af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub const fn run_on_cpu(mut self, cpu: u32) -> Self { | ||
| self.cpu = cpu; | ||
| self.flags |= BPF_F_TEST_RUN_ON_CPU; | ||
| self |
There was a problem hiding this comment.
Remove unusable run_on_cpu option from TestRunAttrs
TestRunAttrs::run_on_cpu is exposed on TestRunOptions, but RawTracePoint (the only documented supported type) uses RawTracePointRunOptions instead (impl TestRun for RawTracePoint). This means users cannot apply this option where docs say it works, and using it on current TestRunOptions impls can fail at runtime with EINVAL.
Useful? React with 👍 / 👎.
|
LLM findings seem legit, can you please address? |
hsqStephenZhang
left a comment
There was a problem hiding this comment.
done.
@hsqStephenZhang made 4 comments and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on hsqStephenZhang).
| pub const fn run_on_cpu(mut self, cpu: u32) -> Self { | ||
| self.cpu = cpu; | ||
| self.flags |= BPF_F_TEST_RUN_ON_CPU; | ||
| self |
| /// Sets the CPU to run the test on. | ||
| /// | ||
| /// This automatically sets the `BPF_F_TEST_RUN_ON_CPU` flag. | ||
| /// This option only works with `RawTracePoint` programs. | ||
| #[must_use] | ||
| pub const fn run_on_cpu(mut self, cpu: u32) -> Self { | ||
| self.cpu = cpu; | ||
| self.flags |= BPF_F_TEST_RUN_ON_CPU; | ||
| self |
| /// # Returns | ||
| /// | ||
| /// Returns a [`crate::TestRunResult`] containing: | ||
| /// - `return_value`: The value returned by the program | ||
| /// - `duration`: Execution time in nanoseconds | ||
| /// - `data_size_out`: Size of data written to output buffer | ||
| /// - `ctx_size_out`: Size of context written to output buffer | ||
| /// |
tamird
left a comment
There was a problem hiding this comment.
I don't understand the latest changes. The LLMs were asking for docs changes, but you just removed the run-on-cpu feature?
@tamird reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on hsqStephenZhang).

background
BPF_PROG_TEST_RUN(hereafter referred asTEST_RUN) was introduced in 4.12 and is very convenient for testing. #36 mentioned but it's not yet implemented.It's quite useful especially if you wish to handle complex logics. Cilium had also brought this testing framework to their project.
This PR aims at bring
TEST_RUNto aya to improve the testing experience.implementation
Basically, it's a wrap around
sys_bpfsyscall with optionTEST_RUN.Supported programs
the following program are supported as mentioned here, some type does not have a public api in Aya, so I did not implemented
TEST_RUNfor them either.SocketFilterSchedClassifierSkLookupCgroupSkbFlowDissectorRawTracePointTracePointXdpexample
testing
I add some tests that covers the options for
BPF_PROG_TEST_RUNin integration tests, which also including some explanations on how to config some context for test purpose.references
This change is