linux-kernel - Re: [PATCH v2] kbuild: factor out the common installation code into scripts/install.sh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 10 May 2022 15:21:48 +0200
From:   Nicolas Schier <n.schier@....de>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] kbuild: factor out the common installation code into
 scripts/install.sh

On Tue, May 10, 2022 at 09:56:45PM +0900, Masahiro Yamada wrote:
> On Tue, May 10, 2022 at 7:04 PM Nicolas Schier <n.schier@....de> wrote:
> >
> > On Tue,  3 May 2022 11:47 +0900 Masahiro Yamada wrote:
> > > Many architectures have similar install.sh scripts.
> > >
> > > The first half is really generic; verifies that the kernel image and
> > > System.map exist, then executes ~/bin/${INSTALLKERNEL} or
> > > /sbin/${INSTALLKERNEL} if available.
> > >
> > > The second half is kind of arch-specific. It just copies the kernel image
> > > and System.map to the destination, but the code is slightly different.
> > >
> > > This patch factors out the generic part into scripts/install.sh.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
> > > ---
> > >
> > > Changes in v2:
> > >   - Move the installkernel parameters to scripts/install.sh
> > >
> > >  Makefile                     |  3 ++-
> > >  arch/arm/Makefile            |  4 ++--
> > >  arch/arm/boot/install.sh     | 21 ------------------
> > >  arch/arm64/Makefile          |  6 ++----
> > >  arch/arm64/boot/install.sh   | 21 ------------------
> > >  arch/ia64/Makefile           |  3 ++-
> > >  arch/ia64/install.sh         | 10 ---------
> > >  arch/m68k/Makefile           |  3 ++-
> > >  arch/m68k/install.sh         | 22 -------------------
> > >  arch/nios2/Makefile          |  3 +--
> > >  arch/nios2/boot/install.sh   | 22 -------------------
> > >  arch/parisc/Makefile         | 11 +++++-----
> > >  arch/parisc/install.sh       | 28 ------------------------
> > >  arch/powerpc/Makefile        |  3 +--
> > >  arch/powerpc/boot/install.sh | 23 --------------------
> > >  arch/riscv/Makefile          |  7 +++---
> > >  arch/riscv/boot/install.sh   | 21 ------------------
> > >  arch/s390/Makefile           |  3 +--
> > >  arch/s390/boot/install.sh    |  6 ------
> > >  arch/sparc/Makefile          |  3 +--
> > >  arch/sparc/boot/install.sh   | 22 -------------------
> > >  arch/x86/Makefile            |  3 +--
> > >  arch/x86/boot/install.sh     | 22 -------------------
> > >  scripts/install.sh           | 41 ++++++++++++++++++++++++++++++++++++
> > >  24 files changed, 64 insertions(+), 247 deletions(-)
> > >  mode change 100644 => 100755 arch/arm/boot/install.sh
> > >  mode change 100644 => 100755 arch/arm64/boot/install.sh
> > >  mode change 100644 => 100755 arch/ia64/install.sh
> > >  mode change 100644 => 100755 arch/m68k/install.sh
> > >  mode change 100644 => 100755 arch/nios2/boot/install.sh
> > >  mode change 100644 => 100755 arch/parisc/install.sh
> > >  mode change 100644 => 100755 arch/powerpc/boot/install.sh
> > >  mode change 100644 => 100755 arch/riscv/boot/install.sh
> > >  mode change 100644 => 100755 arch/s390/boot/install.sh
> > >  mode change 100644 => 100755 arch/sparc/boot/install.sh
> > >  mode change 100644 => 100755 arch/x86/boot/install.sh
> > >  create mode 100755 scripts/install.sh
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 9a60f732bb3c..154c32af8805 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1298,7 +1298,8 @@ scripts_unifdef: scripts_basic
> > >  # to this Makefile to build and install external modules.
> > >  # Cancel sub_make_done so that options such as M=, V=, etc. are parsed.
> > >
> > > -install: sub_make_done :=
> > > +quiet_cmd_install = INSTALL $(INSTALL_PATH)
> > > +      cmd_install = unset sub_make_done; $(srctree)/scripts/install.sh
> >
> > This is the third 'cmd_install' in the tree; might it be better to take
> > a more unique name (e.g. cmd_installkernel) to prevent confusion?
> 
> If this is confusing, we can rename the ones in
> scripts/Makefile.{modinst,headersinst}.
> 
> This command name matches the build target ("make install"), so
> I believe this name is good.
> 
> 
> 
> >
> > For me, it would have been more clear, if we'd also move the default
> > KBUILD_IMAGE definition here (similar to the corresponding part in
> > arch/parisc/Makefile):
> >
> > zinstall: KBUILD_IMAGE := $(boot)/Image.gz
> >
> > ($(KBUILD_IMAGE) seems not to be used anywhere else in arch/arm64/
> > tree; but I haven't checked in depth.)
> 
> 
> KBUILD_IMAGE is _indirectly_ used to specify
> the kernel image for package builds.    [1]
> 
> This target returns the value IMAGE_BUILD.   [2]
> 
> 
> $(KBUILD_IMAGE) is definitely used for arm64.
> 
> [1]: https://github.com/torvalds/linux/blob/v5.17/scripts/package/builddeb#L150
> [2]: https://github.com/torvalds/linux/blob/v5.17/Makefile#L1943
> 
> 
> 
> 
> 
> 
> 
> > > diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
> > > index 7583fc39ab2d..aca1710fd658 100644
> > > --- a/arch/parisc/Makefile
> > > +++ b/arch/parisc/Makefile
> > > @@ -184,12 +184,11 @@ vdso_install:
> > >       $(Q)$(MAKE) $(build)=arch/parisc/kernel/vdso $@
> > >       $(if $(CONFIG_COMPAT_VDSO), \
> > >               $(Q)$(MAKE) $(build)=arch/parisc/kernel/vdso32 $@)
> > > -install:
> > > -     $(CONFIG_SHELL) $(srctree)/arch/parisc/install.sh \
> > > -                     $(KERNELRELEASE) vmlinux System.map "$(INSTALL_PATH)"
> > > -zinstall:
> > > -     $(CONFIG_SHELL) $(srctree)/arch/parisc/install.sh \
> > > -                     $(KERNELRELEASE) vmlinuz System.map "$(INSTALL_PATH)"
> > > +
> > > +install: KBUILD_IMAGE := vmlinux
> > > +zinstall: KBUILD_IMAGE := vmlinuz
> >
> > Does this make the KBUILD_IMAGE definition in line 19 obsolete and
> > unused?
> 
> 
> As I said above for arm64, KBUILD_IMAGE is used for package builds.
> 
> If you delete line 19, it will change the behavior.
> (fall back to the global default [3])
> 
> 
> [3]: https://github.com/torvalds/linux/blob/v5.17/Makefile#L1059
> 
> 
> 
> 
> 
> > > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> > > index eb541e730d3c..45a9caa37b4e 100644
> > > --- a/arch/powerpc/Makefile
> > > +++ b/arch/powerpc/Makefile
> > > @@ -408,8 +408,7 @@ endef
> > >
> > >  PHONY += install
> > >  install:
> >
> > I can't find a KBUILD_IMAGE definition in arch/powerpc/Makefile.
> > Should it be set here as a target-specific varibable, too?
> 
> 
> Right, powerpc does not define KBUILD_IMAGE explicitly.
> It falls back to [3].
> 

Thanks for all those illuming clarifications, I have nothing more to ask.

Reviewed-by: Nicolas Schier <n.schier@....de>