| Message ID | 20240114080644.5086-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V5,1/2] rpm-pkg: simplify installkernel %post | expand |
On Sun, Jan 14, 2024 at 5:07 PM Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote: > > The new installkernel application that is now included in systemd-udev > package allows installation although destination files are already present > in the boot directory of the kernel package, but is failing with the > implemented workaround for the old installkernel application from grubby > package. > > For the new installkernel application, as Davide says: > <<The %post currently does a shuffling dance before calling installkernel. > This isn't actually necessary afaict, and the current implementation > ends up triggering downstream issues such as > https://github.com/systemd/systemd/issues/29568 > This commit simplifies the logic to remove the shuffling. For reference, > the original logic was added in commit 3c9c7a14b627("rpm-pkg: add %post > section to create initramfs and grub hooks").>> > > But we need to keep the old behavior as well, because the old installkernel > application from grubby package, does not allow this simplification and > we need to be backward compatible to avoid issues with the different > packages. > > Mimic Fedora shipping process and store vmlinuz, config amd System.map > in the module directory instead of the boot directory. In this way, we will > avoid the commented problem for all the cases, because the new destination > files are not going to exist in the boot directory of the kernel package. > > Replace installkernel tool with kernel-install tool, because the latter is > more complete. > > Besides, after installkernel tool execution, check to complete if suitable > (same release and build) vmlinuz, System.map and config files are present > in /boot directory, and if necessary, copy manually for install operation > or remmove manually for remove operation. > > Tested with Fedora 38, Fedora 39, RHEL 9, Oracle Linux 9.3, > openSUSE Tumbleweed and openMandrive ROME, using dnf/zypper and rpm tools. > > cc: stable@vger.kernel.org > Co-Developed-by: Davide Cavalca <dcavalca@meta.com> > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> > --- > V1 -> V2: > - Complete to be backward compatible with the previous installkernel > application. > V2 -> V3: > - Follow the suggestions from Masahiro Yamada and change the installation > destination to avoid problems instead of checking the package. > V3 -> V4: > - Make the patch applicable to linux-kbuild/for-next (ia64 support was > already removed). > V4 -> V5: > - Complete for other Linux distributions. > > scripts/package/kernel.spec | 39 +++++++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec > index 89298983a169..74542af8cbfe 100644 > --- a/scripts/package/kernel.spec > +++ b/scripts/package/kernel.spec > @@ -55,12 +55,12 @@ patch -p1 < %{SOURCE2} > %{make} %{makeflags} KERNELRELEASE=%{KERNELRELEASE} KBUILD_BUILD_VERSION=%{release} > > %install > -mkdir -p %{buildroot}/boot > -cp $(%{make} %{makeflags} -s image_name) %{buildroot}/boot/vmlinuz-%{KERNELRELEASE} > +mkdir -p %{buildroot}/lib/modules/%{KERNELRELEASE} > +cp $(%{make} %{makeflags} -s image_name) %{buildroot}/lib/modules/%{KERNELRELEASE}/vmlinuz > %{make} %{makeflags} INSTALL_MOD_PATH=%{buildroot} modules_install > %{make} %{makeflags} INSTALL_HDR_PATH=%{buildroot}/usr headers_install > -cp System.map %{buildroot}/boot/System.map-%{KERNELRELEASE} > -cp .config %{buildroot}/boot/config-%{KERNELRELEASE} > +cp System.map %{buildroot}/lib/modules/%{KERNELRELEASE} > +cp .config %{buildroot}/lib/modules/%{KERNELRELEASE}/config > ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEASE}/build > %if %{with_devel} > %{make} %{makeflags} run-command KBUILD_RUN_COMMAND='${srctree}/scripts/package/install-extmod-build %{buildroot}/usr/src/kernels/%{KERNELRELEASE}' > @@ -70,19 +70,35 @@ ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEA > rm -rf %{buildroot} > > %post > -if [ -x /sbin/installkernel -a -r /boot/vmlinuz-%{KERNELRELEASE} -a -r /boot/System.map-%{KERNELRELEASE} ]; then > -cp /boot/vmlinuz-%{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm > -cp /boot/System.map-%{KERNELRELEASE} /boot/.System.map-%{KERNELRELEASE}-rpm > -rm -f /boot/vmlinuz-%{KERNELRELEASE} /boot/System.map-%{KERNELRELEASE} > -/sbin/installkernel %{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm > -rm -f /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm > +if [ -x /usr/bin/kernel-install ]; then > +/usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNELRELEASE}/vmlinuz > fi > +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then > +release_match=0 > +else > +release_match=1 > +fi > +for file in vmlinuz System.map config; do > +if [ ! -e /boot/${file}-%{KERNELRELEASE} ] || [ ${release_match} != 0 ]; then > +cp -v /lib/modules/%{KERNELRELEASE}/${file} /boot/${file}-%{KERNELRELEASE} > +fi > +done > > %preun > if [ -x /sbin/new-kernel-pkg ]; then > new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=/boot/initramfs-%{KERNELRELEASE}.img > elif [ -x /usr/bin/kernel-install ]; then > -kernel-install remove %{KERNELRELEASE} > +/usr/bin/kernel-install remove %{KERNELRELEASE} > +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then I do not understand why this is needed. Please explain. And, is the output of 'file' standardized? You need to understand that ARCH is not always x86, and /boot/vmlinuz-%{KERNELRELEASE} is not always arch/x86/boot/bzImage. See arch/arm64/Makefile KBUILD_IMAGE := $(boot)/Image.gz For arm64, /boot/vmlinuz-%{KERNELRELEASE} is Image.gz 'file' says it is gzip data, that's all. You cannot read the build version. > +release_match=0 > +else > +release_match=1 > +fi > +for file in vmlinuz System.map config; do > +if [ -e /boot/${file}-%{KERNELRELEASE} ] && [ ${release_match} == 0 ]; then > +rm -v /boot/${file}-%{KERNELRELEASE} > +fi > +done Unreadable. I suggested code with indentation and quotation, but you got rid of them. > fi > > %postun > @@ -94,7 +110,6 @@ fi > %defattr (-, root, root) > /lib/modules/%{KERNELRELEASE} > %exclude /lib/modules/%{KERNELRELEASE}/build > -/boot/* > > %files headers > %defattr (-, root, root) > -- > 2.43.0 >
>> %post >> ... >> +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then >> ... >> >> %preun ... >> +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then > I do not understand why this is needed. > Please explain. Of course. Fisrt of all, I have seen (i.e. openSUSE Tumbleweed) that in the same way that vmlinuz, System.map and config was not copied when the rpm was installed (because of the reason that you commented with the missing script), they were not removed when the rpm was removed, so I have added the lines to remove in a similar way as you suggested for install. And I have seen as well (i.e. openSUSE Tumbleweed)) that if the a new rpm is installed (same release but bigger build version to use default options for the tool), vmlinuz, System.map and config are not copied from %post because vmlinuz, System.map and config already exist and the situation is not good, because /lib/modules/{KERNELRELEASE} is updated but the commented files in /boot are not updated. That is the reason why I have tried to identify when vmlinuz, System.map and config are not the good ones, to copy too. Besides, in the commented situation, the older rpm (same release but older build version) is removed and with that, the new vmlinuz, System.map and config are removed too. That is the reason that I have tried to identify again the files, removing only the suitable vmlinuz, System.map and config with the same release and build number requested. > And, is the output of 'file' standardized? With no more information, file is going to print the strings in the file, that is, the information containig release, version, ... and here we can find what we are interested in. So in some way depends on vmlinuz binary. > You need to understand that ARCH is not always x86, > and /boot/vmlinuz-%{KERNELRELEASE} > is not always arch/x86/boot/bzImage. > > See arch/arm64/Makefile KBUILD_IMAGE := $(boot)/Image.gz > > For arm64, /boot/vmlinuz-%{KERNELRELEASE} is Image.gz > > 'file' says it is gzip data, that's all. > You cannot read the build version. You are right, again good catch. I will try to think something for aarch64. Maybe something more general, and independent of the kernel binary name, is possible and valid for other architectures, maybe with rpm command. If nothing comes up, I will do only for x86. > Unreadable. > I suggested code with indentation and quotation, > but you got rid of them. I did not want to modify the style. Ok, I will follow your suggestion, it's clearer to me too. Thanks Best regards José Ignacio
On Thu, Jan 18, 2024 at 11:12 PM Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote: > > >> %post > >> ... > >> +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then > >> ... > >> > >> %preun > ... > >> +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then > > I do not understand why this is needed. > > Please explain. > Of course. > Fisrt of all, I have seen (i.e. openSUSE Tumbleweed) that in the same way > that vmlinuz, System.map and config was not copied when the rpm was > installed (because of the reason that you commented with the missing > script), they were not removed when the rpm was removed, so I have added > the lines to remove in a similar way as you suggested for install. Here, you are wrong. Those installed files should be removed by %ghost markers. I already have a local patch to do this. (see the attachment) I just asked you to fix up the code as I suggested in v4. > And I have seen as well (i.e. openSUSE Tumbleweed)) that if the a new rpm > is installed (same release but bigger build version to use default options > for the tool), vmlinuz, System.map and config are not copied from %post > because vmlinuz, System.map and config already exist and the situation is > not good, because /lib/modules/{KERNELRELEASE} is updated but the commented > files in /boot are not updated. That is the reason why I have tried to > identify when vmlinuz, System.map and config are not the good ones, to copy > too. For me (on Fedora 39 and openSUSE Tumbleweed), rpm fails due to file conflict. vagrant@opensuse-tumbleweed20231218:~> sudo rpm -i kernel-6.7.0_12924_g660a5f4a53e7-4.x86_64.rpm file /lib/modules/6.7.0-12924-g660a5f4a53e7/vmlinuz from install of kernel-6.7.0_12924_g660a5f4a53e7-4.x86_64 conflicts with file from package kernel-6.7.0_12924_g660a5f4a53e7-3.x86_64 So, this does not happen. > Besides, in the commented situation, the older rpm (same release but older > build version) is removed and with that, the new vmlinuz, System.map and > config are removed too. That is the reason that I have tried to identify > again the files, removing only the suitable vmlinuz, System.map and config > with the same release and build number requested. > > > And, is the output of 'file' standardized? > With no more information, file is going to print the strings in the file, > that is, the information containig release, version, ... and here we can > find what we are interested in. So in some way depends on vmlinuz binary. > > > You need to understand that ARCH is not always x86, > > and /boot/vmlinuz-%{KERNELRELEASE} > > is not always arch/x86/boot/bzImage. > > > > See arch/arm64/Makefile > KBUILD_IMAGE := $(boot)/Image.gz > > > > For arm64, /boot/vmlinuz-%{KERNELRELEASE} is Image.gz > > > > 'file' says it is gzip data, that's all. > > You cannot read the build version. > You are right, again good catch. > I will try to think something for aarch64. Maybe something more general, > and independent of the kernel binary name, is possible and valid for other > architectures, maybe with rpm command. > If nothing comes up, I will do only for x86. No. Please do not. > > > Unreadable. > > I suggested code with indentation and quotation, > > but you got rid of them. > I did not want to modify the style. > Ok, I will follow your suggestion, it's clearer to me too. > > Thanks > > Best regards > José Ignacio >
> Those installed files should be removed by %ghost markers. > I already have a local patch to do this. > (see the attachment) I like the idea of your new patch, a lot of things can be fixed in that way. Ok, I will remove the extra code to remove (%preun) in the patch. Just a comment about your patch: for openSUSE /boot/initramfs-* files are called /boot/initrd-* and maybe someone would not require it (i.e. embedded systems). If it is created it is normally removed and it might not be necessary (although I like your idea to control it). > I just asked you to fix up the code as I suggested in v4. Now I understand why no code was added in %preun. Ok, your suggestion was very good, but let me try and explain better with commands what I would like to fix after next point. When I said 'update' wasn't clear, I think. If it doesn't fit with your idea or global usage, I will include your suggestion like it is. > For me (on Fedora 39 and openSUSE Tumbleweed), rpm fails due to file conflict. > > vagrant@opensuse-tumbleweed20231218:~> sudo rpm -i > kernel-6.7.0_12924_g660a5f4a53e7-4.x86_64.rpm > file /lib/modules/6.7.0-12924-g660a5f4a53e7/vmlinuz from install of > kernel-6.7.0_12924_g660a5f4a53e7-4.x86_64 conflicts with file from > package kernel-6.7.0_12924_g660a5f4a53e7-3.x86_64 > > So, this does not happen. I was refering to the cases when zypper is used to install a new kernel with the same release and different build number or when 'rpm -i --replacefiles' is used (in this case it would be necessary to remove the old kernel with 'rpm -e --justdb' too). In this cases we only need the possibility of copying the files from the new package and not only if they don't exist. I have thought about an easy way (no extra or problematic command) and I think I have it. In addition to your suggestion (if the file does not exit in /boot), I will just compare the file in /boot with the file in /lib/modules/%{KERNELRELEASE} and if it is not the same, we allow copying: %post if [ -x /usr/bin/kernel-install ]; then /usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNELRELEASE}/vmlinuz fi for file in vmlinuz System.map config; do if [ ! -e "/boot/${file}-%{KERNELRELEASE}" ] || ! cmp --silent "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}"; then cp "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}" fi done Let me try with a new patch to know your opinion. Thanks Best regards José Ignacio
diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec index 89298983a169..74542af8cbfe 100644 --- a/scripts/package/kernel.spec +++ b/scripts/package/kernel.spec @@ -55,12 +55,12 @@ patch -p1 < %{SOURCE2} %{make} %{makeflags} KERNELRELEASE=%{KERNELRELEASE} KBUILD_BUILD_VERSION=%{release} %install -mkdir -p %{buildroot}/boot -cp $(%{make} %{makeflags} -s image_name) %{buildroot}/boot/vmlinuz-%{KERNELRELEASE} +mkdir -p %{buildroot}/lib/modules/%{KERNELRELEASE} +cp $(%{make} %{makeflags} -s image_name) %{buildroot}/lib/modules/%{KERNELRELEASE}/vmlinuz %{make} %{makeflags} INSTALL_MOD_PATH=%{buildroot} modules_install %{make} %{makeflags} INSTALL_HDR_PATH=%{buildroot}/usr headers_install -cp System.map %{buildroot}/boot/System.map-%{KERNELRELEASE} -cp .config %{buildroot}/boot/config-%{KERNELRELEASE} +cp System.map %{buildroot}/lib/modules/%{KERNELRELEASE} +cp .config %{buildroot}/lib/modules/%{KERNELRELEASE}/config ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEASE}/build %if %{with_devel} %{make} %{makeflags} run-command KBUILD_RUN_COMMAND='${srctree}/scripts/package/install-extmod-build %{buildroot}/usr/src/kernels/%{KERNELRELEASE}' @@ -70,19 +70,35 @@ ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEA rm -rf %{buildroot} %post -if [ -x /sbin/installkernel -a -r /boot/vmlinuz-%{KERNELRELEASE} -a -r /boot/System.map-%{KERNELRELEASE} ]; then -cp /boot/vmlinuz-%{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm -cp /boot/System.map-%{KERNELRELEASE} /boot/.System.map-%{KERNELRELEASE}-rpm -rm -f /boot/vmlinuz-%{KERNELRELEASE} /boot/System.map-%{KERNELRELEASE} -/sbin/installkernel %{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm -rm -f /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm +if [ -x /usr/bin/kernel-install ]; then +/usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNELRELEASE}/vmlinuz fi +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then +release_match=0 +else +release_match=1 +fi +for file in vmlinuz System.map config; do +if [ ! -e /boot/${file}-%{KERNELRELEASE} ] || [ ${release_match} != 0 ]; then +cp -v /lib/modules/%{KERNELRELEASE}/${file} /boot/${file}-%{KERNELRELEASE} +fi +done %preun if [ -x /sbin/new-kernel-pkg ]; then new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=/boot/initramfs-%{KERNELRELEASE}.img elif [ -x /usr/bin/kernel-install ]; then -kernel-install remove %{KERNELRELEASE} +/usr/bin/kernel-install remove %{KERNELRELEASE} +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KERNELRELEASE} | grep -q " #%{release} "; then +release_match=0 +else +release_match=1 +fi +for file in vmlinuz System.map config; do +if [ -e /boot/${file}-%{KERNELRELEASE} ] && [ ${release_match} == 0 ]; then +rm -v /boot/${file}-%{KERNELRELEASE} +fi +done fi %postun @@ -94,7 +110,6 @@ fi %defattr (-, root, root) /lib/modules/%{KERNELRELEASE} %exclude /lib/modules/%{KERNELRELEASE}/build -/boot/* %files headers %defattr (-, root, root)
The new installkernel application that is now included in systemd-udev package allows installation although destination files are already present in the boot directory of the kernel package, but is failing with the implemented workaround for the old installkernel application from grubby package. For the new installkernel application, as Davide says: <<The %post currently does a shuffling dance before calling installkernel. This isn't actually necessary afaict, and the current implementation ends up triggering downstream issues such as https://github.com/systemd/systemd/issues/29568 This commit simplifies the logic to remove the shuffling. For reference, the original logic was added in commit 3c9c7a14b627("rpm-pkg: add %post section to create initramfs and grub hooks").>> But we need to keep the old behavior as well, because the old installkernel application from grubby package, does not allow this simplification and we need to be backward compatible to avoid issues with the different packages. Mimic Fedora shipping process and store vmlinuz, config amd System.map in the module directory instead of the boot directory. In this way, we will avoid the commented problem for all the cases, because the new destination files are not going to exist in the boot directory of the kernel package. Replace installkernel tool with kernel-install tool, because the latter is more complete. Besides, after installkernel tool execution, check to complete if suitable (same release and build) vmlinuz, System.map and config files are present in /boot directory, and if necessary, copy manually for install operation or remmove manually for remove operation. Tested with Fedora 38, Fedora 39, RHEL 9, Oracle Linux 9.3, openSUSE Tumbleweed and openMandrive ROME, using dnf/zypper and rpm tools. cc: stable@vger.kernel.org Co-Developed-by: Davide Cavalca <dcavalca@meta.com> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- V1 -> V2: - Complete to be backward compatible with the previous installkernel application. V2 -> V3: - Follow the suggestions from Masahiro Yamada and change the installation destination to avoid problems instead of checking the package. V3 -> V4: - Make the patch applicable to linux-kbuild/for-next (ia64 support was already removed). V4 -> V5: - Complete for other Linux distributions. scripts/package/kernel.spec | 39 +++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)