-
Notifications
You must be signed in to change notification settings - Fork 255
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
aya: expose BPF verifier log level configuration #371
Conversation
✅ Deploy Preview for aya-rs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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 doing this! Look good, see enum comment
aya/src/bpf.rs
Outdated
/// # Ok::<(), aya::BpfError>(()) | ||
/// ``` | ||
/// | ||
pub fn verifier_log_level(&mut self, level: u32) -> &mut BpfLoader<'a> { |
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.
I think instead of u32
we should probably have an enum:
#[non_exhaustive]
#[repr(u32)]
pub enum VerifierLogLevel {
Default = 4,
Verbose = 1,
Debug = 7,
}
See https://github.com/torvalds/linux/blob/f16214c102f0f64b2f3546e989498525bd7b7708/include/linux/bpf_verifier.h#L419. The Default/Verbose value correspond to what libbpf does too.
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.
Updated it here, changed the names and added some valid configurations.
Let me know if you still prefer the above variants, I just felt like this it was slightly clearer.
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.
Could also change it up so that is similar to a bitmap, e.g. set it up like:
VerifierLogLevel::default().level_1(true).stats(true)
But might be too verbose for something like this.
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.
Updated it here, changed the names and added some valid configurations.
Let me know if you still prefer the above variants, I just felt like this it was slightly clearer.
I don't think this is cleaner - in fact it took me a good 10 minutes yesterday to understand why those names were used in the kernel and how they work. The names are bad, and I don't think we should surface the fact that log level s are implemented as a bitfield in the kernel all the way up to the aya API.
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.
We should also have a Disable = 0
variant in addition to the ones I initially suggested tho
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.
Looks great, thank you!
Hi!
Simple change that allows configuring verifier log level using
BpfLoader
.Reducing the log level is very useful when debugging since the last line of the verifier error is its cause, so when the buffer is out of space you can't see the cause.
For reference: change discussed here