fix(aya): Fix panic when creating map on custom ubuntu kernel #909
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
aya/src/util.rs
Outdated
| @@ -130,7 +130,7 @@ impl KernelVersion { | |||
| } | |||
|
|
|||
| fn get_kernel_version() -> Result<Self, CurrentKernelVersionError> { | |||
| if let Some(v) = Self::get_ubuntu_kernel_version()? { | |||
| if let Some(v) = Self::get_ubuntu_kernel_version().ok().flatten() { | |||
There was a problem hiding this comment.
I'm wondering if we should do the same (ok().flatten()) in line 142 for consistency as well? Debian kernel check probably works similarly.
There was a problem hiding this comment.
I think the debian check is less fallible since it relies on the standard kernel version
There was a problem hiding this comment.
Feel free to give suggestions on how to implement such a test, and i'll be happy to add it to the PR
There was a problem hiding this comment.
Unit tests would be awesome, but I guess they would trigger a bigger refactoring of this function.
Instead of retrieving the version by calling Self::get_<distro>_kernel_version inside get_kernel_version, we would rather want to have a function which gets the kernel string as an argument and then parses and processes it. This way we could test it in a straightforward way (by passing a string to get_kernel_version() and asserting the output).
On the side note, I'm not a big fan of get_kernel_version() utility in general. After quick grepping, I don't see it being used internally in Aya at all. The whole version parsing per distro is becoming a mess. And using kernel versions for detecting features is not even reliable - mostly because of distros often backporting quite fresh stuff to completely ancient kernels. In the long term, I think we should probably deprecate get_kernel_version() and instead work on some more reliable feature detection, similar to bpftool feature, based on:
- parsing kernel config
- compilation and loading of tiny programs, which just indicate whether a program type X and helper function Y can be used on the currently running system
It would be great to start this effort as an additional crate (outside the monorepo). I have an use-case for this myself, so I'm happy to start it myself, but of course help is welcome.
There was a problem hiding this comment.
Can i make the requested changes by @tamird so that we can have tests, and can we merge this for now? I really need this.
There was a problem hiding this comment.
Yes, a larger refactoring can happen later
There was a problem hiding this comment.
After quick grepping, I don't see it being used internally in Aya at all.
We use KernelVersion::current() which calls it
|
oof, githubs sync fork button striked, i should stop doing prs on my main branches..., i'll try to fix |
|
I don't love that this has no test coverage and uses |
|
Test coverage of which function is missing? |
Currently
aya::maps::MapData::createpanics at this line when it is called on a custom / faulty Ubuntu kernel (tested with Proxmox VE 8.1.4).The file
get_ubuntu_kernel_versionaccesses contains only a new line on Proxmox VE 8.1.4 thusget_ubuntu_kernel_versionreturns a ParseError.This PR changes the behaviour so that it will try the other options if there is a parse error.