Received-SPF: pass (google.com: domain of linux-kernel+bounces-28426-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <CAK7LNAR_wgQBs-q9NH1icb_FPBoVMNEhQpvV8qzH2dFsrDS0pQ@mail.gmail.com>
 <20240114080644.5086-1-jtornosm@redhat.com>
In-Reply-To: <20240114080644.5086-1-jtornosm@redhat.com>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Wed, 17 Jan 2024 10:29:25 +0900
Message-ID: <CAK7LNAQ7C+ZN+mxaiw3-10Lmn2f0nAYi+aou=CWraZwV=_iThw@mail.gmail.com>
Subject: Re: [PATCH V5 1/2] 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 Sun, Jan 14, 2024 at 5:07=E2=80=AFPM 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 suitabl=
e
> (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=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,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-%{KERNELRELE=
ASE}-rpm
> +if [ -x /usr/bin/kernel-install ]; then
> +/usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNELRELEAS=
E}/vmlinuz
>  fi
> +if [ -e /boot/vmlinuz-%{KERNELRELEASE} ] && file -bL /boot/vmlinuz-%{KER=
NELRELEASE} | grep -q " #%{release} "; then
> +release_match=3D0
> +else
> +release_match=3D1
> +fi
> +for file in vmlinuz System.map config; do
> +if [ ! -e /boot/${file}-%{KERNELRELEASE} ] || [ ${release_match} !=3D 0 =
]; then
> +cp -v /lib/modules/%{KERNELRELEASE}/${file} /boot/${file}-%{KERNELRELEAS=
E}
> +fi
> +done
>
>  %preun
>  if [ -x /sbin/new-kernel-pkg ]; then
>  new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=3D/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-%{KER=
NELRELEASE} | 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    :=3D $(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=3D0
> +else
> +release_match=3D1
> +fi
> +for file in vmlinuz System.map config; do
> +if [ -e /boot/${file}-%{KERNELRELEASE} ] && [ ${release_match} =3D=3D 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
>


--=20
Best Regards
Masahiro Yamada