Received-SPF: pass (google.com: domain of linux-kernel+bounces-41557-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <CAK7LNAQCiBtQ3kQznPDKtkD83wpCzodPVDs8eFnfnx5=Y8E5Cw@mail.gmail.com>
 <20240122182253.17973-1-jtornosm@redhat.com>
In-Reply-To: <20240122182253.17973-1-jtornosm@redhat.com>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Sun, 28 Jan 2024 16:32:33 +0900
Message-ID: <CAK7LNAR2xDjbn+BZqUrgbDxPJUyQBULFB51kTiN8Nc78DXVyEw@mail.gmail.com>
Subject: Re: [PATCH] rpm-pkg: simplify installkernel %post
To: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Cc: dcavalca@meta.com, linux-kbuild@vger.kernel.org, 
	linux-kernel@vger.kernel.org, nathan@kernel.org, ndesaulniers@google.com, 
	nicolas@fjasle.eu, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 23, 2024 at 3:23=E2=80=AFAM 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 presen=
t
> 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 installkern=
el
> 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 wi=
ll
> avoid the commented problem for all the cases, because the new destinatio=
n
> files are not going to exist in the boot directory of the kernel package.
>
> Replace installkernel tool with kernel-install tool, because the latter i=
s
> more complete.
>
> Besides, after installkernel tool execution, check to complete if the
> correct package files vmlinuz, System.map and config files are present
> in /boot directory, and if necessary, copy manually for install operation=
.
> In this way, take into account if  files were not previously copied from
> /usr/lib/kernel/install.d/* scripts and if the suitable files for the
> requested package are present (it could be others if the rpm files were
> replace with a new pacakge with the same release and a different build).
>
> 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.
> V5 -> V6
> - Simplify and do more compatible checks when copied files wants to be
>   replaced.
> - Remove %preun because it will be better done with another patch.
> - Add indentation and quotation
>
>  scripts/package/kernel.spec | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec
> index 89298983a169..0bff257ec3d4 100644
> --- a/scripts/package/kernel.spec
> +++ b/scripts/package/kernel.spec
> @@ -55,12 +55,12 @@ patch -p1 < %{SOURCE2}
>  %{make} %{makeflags} KERNELRELEASE=3D%{KERNELRELEASE} KBUILD_BUILD_VERSI=
ON=3D%{release}
>
>  %install
> -mkdir -p %{buildroot}/boot
> -cp $(%{make} %{makeflags} -s image_name) %{buildroot}/boot/vmlinuz-%{KER=
NELRELEASE}
> +mkdir -p %{buildroot}/lib/modules/%{KERNELRELEASE}
> +cp $(%{make} %{makeflags} -s image_name) %{buildroot}/lib/modules/%{KERN=
ELRELEASE}/vmlinuz
>  %{make} %{makeflags} INSTALL_MOD_PATH=3D%{buildroot} modules_install
>  %{make} %{makeflags} INSTALL_HDR_PATH=3D%{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/%{KER=
NELRELEASE}/build
>  %if %{with_devel}
>  %{make} %{makeflags} run-command KBUILD_RUN_COMMAND=3D'${srctree}/script=
s/package/install-extmod-build %{buildroot}/usr/src/kernels/%{KERNELRELEASE=
}'
> @@ -70,31 +70,31 @@ 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-%{KERNELRELE=
ASE}-rpm
> +if [ -x /usr/bin/kernel-install ]; then
> +       /usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNE=
LRELEASE}/vmlinuz
>  fi
> +for file in vmlinuz System.map config; do
> +       if [ ! -e "/boot/${file}-%{KERNELRELEASE}" ] || ! cmp --silent "/=
lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}"; the=
n
> +               cp "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}=
-%{KERNELRELEASE}"
> +       fi
> +done


I am fine with this approach, but
[ ! -e "/boot/${file}-%{KERNELRELEASE}" ]
is a redundant check now.


When the file does not exist, "cmp --silent" exits
with 2 instead of 1.
Anyway, "cmp --silent" fails.


You can simplify the conditional to:

if ! cmp --silent "/lib/modules/%{KERNELRELEASE}/${file}"
"/boot/${file}-%{KERNELRELEASE}"; then


>  %preun
>  if [ -x /sbin/new-kernel-pkg ]; then
> -new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=3D/boot=
/initramfs-%{KERNELRELEASE}.img
> +       new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=
=3D/boot/initramfs-%{KERNELRELEASE}.img

This is a noise change.
Please drop.


>  elif [ -x /usr/bin/kernel-install ]; then
> -kernel-install remove %{KERNELRELEASE}
> +       /usr/bin/kernel-install remove %{KERNELRELEASE}
>  fi
>
>  %postun
>  if [ -x /sbin/update-bootloader ]; then
> -/sbin/update-bootloader --remove %{KERNELRELEASE}
> +       /sbin/update-bootloader --remove %{KERNELRELEASE}

Ditto.

Please create a separate patch if you change the coding style.
But, rather, I am thinking of reverting
6ef41e22a320 and 27c3bffd230ab


>  fi
>
>  %files
>  %defattr (-, root, root)
>  /lib/modules/%{KERNELRELEASE}
>  %exclude /lib/modules/%{KERNELRELEASE}/build
> -/boot/*
>
>  %files headers
>  %defattr (-, root, root)
> --
> 2.43.0
>


--=20
Best Regards
Masahiro Yamada