-
Notifications
You must be signed in to change notification settings - Fork 164
Introduce BPF Streams abstraction #1284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
danielocfb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thank you for the review, I will update the PR accordingly and post here to confirm it's ready for review. Ack to all comments, though I have a couple followup questions (inline). |
|
Updated the code and added tests. The interface now presents the streams only indirectly and without exposing the type itself unless the user opts to create an instance directly. I think the code now is more idiomatic and looks a lot less like C. |
ad9d89b to
2b0bad2
Compare
|
The failing tests looks like are caused the test VMs' kernel being < 6.17? |
danielocfb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now. Left a few more coments/questions/suggestions; please take another look. Please also fix CI so we can merge.
libbpf-rs/src/streams.rs
Outdated
| libbpf_sys::bpf_prog_stream_read( | ||
| self.prog_fd.as_raw_fd(), | ||
| self.stream_id, | ||
| buf.as_mut_ptr() as *mut c_void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| buf.as_mut_ptr() as *mut c_void, | |
| buf.as_mut_ptr().cast() |
libbpf-rs/src/streams.rs
Outdated
| self.stream_id, | ||
| buf.as_mut_ptr() as *mut c_void, | ||
| buf.len().try_into().unwrap(), | ||
| &mut c_opts as *mut bpf_prog_stream_read_opts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| &mut c_opts as *mut bpf_prog_stream_read_opts, | |
| &raw mut c_opts, |
| let mut stdout = prog.stdout(); | ||
| let mut buf = [0u8; 1024]; | ||
|
|
||
| // The read itself should succeed and return 0 bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I misreading or does the comment state the opposite of what is being asserted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, the condition in the assert!() was wrong.
libbpf-rs/tests/bin/src/stream.bpf.c
Outdated
|
|
||
| volatile u64 i; | ||
|
|
||
| /* Trigger a may_goto timeout to emit a streams error. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this may_goto timeout is a way to force the kernel to emit data to stderr, is that how it works? And there is no way to write to a stream directly? And what are the requirements for may_goto to be available? Is it just a certain LLVM version? Or kernel config? It's also not exactly clear what happens if it's not available. Will the test still work?
If there is no easier way to trigger a stream write, can you please expand the comment a bit to make it clear who does what and in response to what exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah pretty much, unfortunatley there isn't a straightforward way to directly emit to stdout/stderr. Only stderr can contain data, and that is generated by the BPF runtime in the kernel rather than the program itself. I'll expand the comment to explain.
For may_goto: It's been available since 6.9, and it works regardless of compiler version because there are compatiblity macros to handle older LLVM versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use bpf_stream_printk (convenience macro over bpf_stream_vprintk) to write to either stream, triggering a may_goto timeout error also means a 250ms delay.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to remove spurious formatting changes. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I ran rustfmt on the file directly and there seem to be some extra newlines already present in the file. I'll remove them from the diff.
If that's the case you can mark them as ignored for now. libbpf-rs/libbpf-rs/tests/test.rs Line 1702 in e0caff4
At the moment we just use whatever kernel the Ubuntu image ships with. Right now that seems to be 6.11 (https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md) |
Add an abstraction for reading BPF Streams. This commit introduces the wrappers around the libbpf-sys method. Signed-off-by: Emil Tsalapatis <[email protected]>
Add tests for BPF Streams. The tests access both the stdout and stderr streams, and ensure the code works regardless of whether there is ready data. Signed-off-by: Emil Tsalapatis <[email protected]>
|
Clippy seems to be failing on unrelated code. |
|
Not sure why clippy is hitting these errors, they are for pre-existing code. |
|
It's failing because you don't use all functions provided by the I fixed it up and merged. Thanks for the pull request! |
|
Thank you for the detailed feedback! |
Add a CHANGELOG entry for pull request libbpf#1284, which added support for accessing BPF stdout and stderr streams. Signed-off-by: Daniel Müller <[email protected]>
Add a CHANGELOG entry for pull request #1284, which added support for accessing BPF stdout and stderr streams. Signed-off-by: Daniel Müller <[email protected]>
Add libbpf-rs support for interacting with BPF streams from Rust code. The interface provides the option to either directly specify the prog fd/stream id, or to hide the details behind a Stream abstraction.