core: Get the kernel version from the kernel path #4038
Conversation
|
Hi @akihikodaki. Thanks for your PR. I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Ah, neat. I am really happy to help the workflow of testing out direct local/upstream kernel builds. Supporting the upstream RPM flow is great. I'd eventually like to take it a step farther and support directly adding a local kernel build without going through RPM at all (it's silly and inefficient to make a compressed RPM, only to immediately decompress it to install). That said though...hmm, ideally we could convince the upstream (I had to go look, the relevant code is https://github.com/torvalds/linux/blob/master/scripts/package/mkspec for reference) Anyways on the actual code changes here...it's not at all your fault but this code was a mess before...I'd been actually planning to replace it with https://github.com/ostreedev/ostree-rs-ext/blob/main/lib/src/bootabletree.rs relatively soon.
So what's going on here I believe is we're moving the files from (Not opposed to merging this patch as is to be clear) Also thank you so much for the contribution! |
|
@cgwalters I believe that "canonicalization" actually happens. That's why I left the logic to find the kernel in |
src/libpriv/rpmostree-kernel.cxx
Outdated
| return glnx_throw (error, "Multiple vmlinuz- in %s", bootdir); | ||
| return glnx_throw (error, "Multiple vmlinuz%s in %s", out_ksuffix ? "-" : "", bootdir); | ||
| if (out_ksuffix) | ||
| ret_ksuffix = g_strdup (name + (sizeof ("vmlinuz-") - 1)); |
There was a problem hiding this comment.
I'd spell this as just strlen ("vmlinuz-") which is less obscure; sizeof ("vmlinuz-") - 1 requires C programmers to remember that sizeof includes the NUL character and you're subtracting it off. strlen will be constant folded by all decent compilers in the same way as sizeof.
|
/ok-to-test |
Before commit 80bb67c, rpm-ostree required there is only one directory in /usr/lib/modules and the directory represents the installed kernel. However, it was revealed that 3rd-party RPMs can have pre-built modules for several kernel versions, and the requirement prevented from installing them. The commit was intended to solve the problem by ignoring directories which do not contain a file named vmlinuz. However, the upstream kernel actually produces RPMs without vmlinuz in /usr/lib/modules, and the logic introduced by the commit prevented them from being installed. This change replaces the logic with yet another alternative that retrieves the kernel version from the kernel file name in the boot directory. The kernel version can be used to identify the kernel module directory without requiring that there is only one directory in /usr/lib/modules or that the directory contains vmlinuz. An old comment in find_kernel_and_initramfs_in_bootdir says the kernel installed in Fedora 23 does not contain the version in the path, but it should not be a problem; looking at the source code, the path has always contained the version since Fedora 12.
There was a problem hiding this comment.
I have some lingering uncertainty about this because it's complex code and we don't have unit tests directly covering it, but we do cover all this in integration tests - our CI is pretty good here.
I'd still like to move this logic to Rust at some point soon.
At some point we can look at adding an upstream-built kernel RPM to our CI flow.
Thanks again for the patch!
Before commit 80bb67c, rpm-ostree required there is only one directory in
/usr/lib/modulesand the directory represents the installed kernel. However, it was revealed that 3rd-party RPMs can have pre-built modules for several kernel versions, and the requirement prevented from installing them. The commit was intended to solve the problem by ignoring directories which do not contain a file namedvmlinuz.However, the upstream kernel actually produces RPMs without
vmlinuzin/usr/lib/modules, and the logic introduced by the commit prevented them from being installed.This change replaces the logic with yet another alternative that retrieves the kernel version from the kernel file name in the boot directory. The kernel version can be used to identify the kernel module directory without requiring that there is only one directory in
/usr/lib/modulesor that the directory containsvmlinuz.An old comment in
find_kernel_and_initramfs_in_bootdirsays the kernel installed in Fedora 23 does not contain the version in the path, but it should not be a problem; looking at the source code, the path has always contained the version since Fedora 12.