Re: [PATCH v3] depmod: Handle installing modules under a prefix

From: Nicolas Schier
Date: Fri Jul 14 2023 - 15:37:44 EST

Next message: Jonathan Corbet: "Re: [PATCH] docs: consolidate core subsystems"
Previous message: Jonathan Corbet: "Re: [PATCH] docs: deprecated.rst: Update an example"
In reply to: Michal Suchánek: "Re: [PATCH v3] depmod: Handle installing modules under a prefix"
Next in thread: Michal Suchanek: "[PATCH kmod v2 1/4] man/depmod.d: Fix incorrect /usr/lib search path"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jul 14, 2023 at 05:10:42PM +0200 Michal Suchánek wrote:
> On Fri, Jul 14, 2023 at 04:54:49PM +0200, Nicolas Schier wrote:
> > On Fri, Jul 14, 2023 at 04:30:02PM +0200, Michal Such�nek wrote:
> > > Hello,
> > >
> > > On Fri, Jul 14, 2023 at 04:05:10PM +0200, Nicolas Schier wrote:
> > > > On Fri, Jul 14, 2023 at 02:21:08PM +0200 Michal Suchanek wrote:
> > > > > Some distributions aim at not shipping any files in / outside of usr.
> > > >
> > > > For me, preventing negation often makes things easier, e.g.: "... aim at
> > > > shipping files only below /usr".
> > > >
> > > > >
> > > > > The path under which kernel modules are installed is hardcoded to /lib
> > > > > which conflicts with this goal.
> > > > >
> > > > > When kmod provides the config command, use it to determine the correct
> > > > > module installation prefix.
> > > > >
> > > > > This is a prefix under which the modules are searched by kmod on the
> > > > > system, and is separate from the temporary staging location already
> > > > > supported by INSTALL_MOD_PATH.
> > > > >
> > > > > With kmod that does not provide the config command empty prefix is used
> > > > > as before.
> > > > >
> > > > > Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx>
> > > > > ---
> > > > > v2: Avoid error on systems with kmod that does not support config
> > > > > command
> > > > > v3: More verbose commit message
> > > > > ---
> > > > > Makefile | 4 +++-
> > > > > scripts/depmod.sh | 8 ++++----
> > > > > 2 files changed, 7 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index 47690c28456a..b1fea135bdec 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -1165,7 +1165,9 @@ export INSTALL_DTBS_PATH ?= $(INSTALL_PATH)/dtbs/$(KERNELRELEASE)
> > > > > # makefile but the argument can be passed to make if needed.
> > > > > #
> > > > >
> > > > > -MODLIB = $(INSTALL_MOD_PATH)/lib/modules/$(KERNELRELEASE)
> > > > > +export KERNEL_MODULE_PREFIX := $(shell kmod config &> /dev/null && kmod config | jq -r .module_prefix)

oh, should this be 'jq -r .prefix' (w/o ".module") to match your other patches?

> > > >
> > > > All other calls of `jq` that I could find are located at tools/; as this here
> > > > is evaluated on each invocation, this should probably be documented in
> > > > Documentation/process/changes.rst?
> > > >
> > > > (Absence of `jq` will cause error messages, even with CONFIG_MODULES=n.)
> > >
> > > That's a good point.
> > >
> > > >
> > > > > +
> > > > > +MODLIB = $(INSTALL_MOD_PATH)$(KERNEL_MODULE_PREFIX)/lib/modules/$(KERNELRELEASE)
> > > > > export MODLIB
> > > > >
> > > > > PHONY += prepare0
> > > > > diff --git a/scripts/depmod.sh b/scripts/depmod.sh
> > > > > index 3643b4f896ed..88ac79056153 100755
> > > > > --- a/scripts/depmod.sh
> > > > > +++ b/scripts/depmod.sh
> > > > > @@ -27,16 +27,16 @@ fi
> > > > > # numbers, so we cheat with a symlink here
> > > > > depmod_hack_needed=true
> > > > > tmp_dir=$(mktemp -d ${TMPDIR:-/tmp}/depmod.XXXXXX)
> > > > > -mkdir -p "$tmp_dir/lib/modules/$KERNELRELEASE"
> > > > > +mkdir -p "$tmp_dir$KERNEL_MODULE_PREFIX/lib/modules/$KERNELRELEASE"
> > > > > if "$DEPMOD" -b "$tmp_dir" $KERNELRELEASE 2>/dev/null; then
> > > > > - if test -e "$tmp_dir/lib/modules/$KERNELRELEASE/modules.dep" -o \
> > > > > - -e "$tmp_dir/lib/modules/$KERNELRELEASE/modules.dep.bin"; then
> > > > > + if test -e "$tmp_dir$KERNEL_MODULE_PREFIX/lib/modules/$KERNELRELEASE/modules.dep" -o \
> > > > > + -e "$tmp_dir$KERNEL_MODULE_PREFIX/lib/modules/$KERNELRELEASE/modules.dep.bin"; then
> > > > > depmod_hack_needed=false
> > > > > fi
> > > > > fi
> > > >
> > > > I'd like to come back to the statement from Masahiro: Is the check above,
> > > > against some very old versions of depmod [1], the only reason for this patch?
> > > >
> > > > If we could remove that, would
> > > >
> > > > make INSTALL_MOD_PATH="$(kmod config | jq -r .module_prefix)" modules_install
> > > >
> > > > be sufficient?
> > >
> > > No, the INSTALL_MOD_PATH is passed as the -b argument to depmod while
> > > the newly added part is not because it's integral part of where the
> > > modules are installed on the system, and not the staging area path.
> >
> > Ah, thanks. So just for my understanding, could this be a (non-gentle)
> > alternative version of your patch, w/o modifying top-level Makefile?
> >
> > diff --git a/scripts/depmod.sh b/scripts/depmod.sh
> > index 3643b4f896ed..72c819de0669 100755
> > --- a/scripts/depmod.sh
> > +++ b/scripts/depmod.sh
> > @@ -1,4 +1,4 @@
> > -#!/bin/sh
> > +#!/bin/bash
> > # SPDX-License-Identifier: GPL-2.0
> > #
> > # A depmod wrapper used by the toplevel Makefile
> > @@ -23,6 +23,8 @@ if [ -z $(command -v $DEPMOD) ]; then
> > exit 0
> > fi
> >
> > +kmod_version=$(( $(kmod --version | sed -rne 's/^kmod version ([0-9]+).*$/\1/p') ))
> > +
> > # older versions of depmod require the version string to start with three
> > # numbers, so we cheat with a symlink here
> > depmod_hack_needed=true
> > @@ -35,6 +37,13 @@ if "$DEPMOD" -b "$tmp_dir" $KERNELRELEASE 2>/dev/null; then
> > fi
> > fi
> > rm -rf "$tmp_dir"
> > +
> > +if [ "${kmod_version}" -gt 32 ]; then
> > + kmod_prefix="$(kmod config | jq -r .module_prefix)"
> > + INSTALL_MOD_PATH="${INSTALL_MOD_PATH#${kmod_prefix}"
> > + depmod_hack_needed=false
> > +fi
> > +
> > if $depmod_hack_needed; then
> > symlink="$INSTALL_MOD_PATH/lib/modules/99.98.$KERNELRELEASE"
> > ln -s "$KERNELRELEASE" "$symlink"
> >
> > (untested, and assuming that kmod module prefix is in kmod >= 32)
>
> It can be detected by running the 'kmod config' command first and
> ignoring the output when it fails which the above patch already did.
> The version check does not sound very reliable.
>
> > Or are I am still missing something?
>
> MODLIB still needs to include the extra prefix so that files are
> installed in the correct location. And that's defined in the toplevel
> Makefile.

Well, I think that depends. Technically, you are right; and if we want
to support system with a non-empty kmod prefix fully transparently, then
patching top-level Makefile will probably be necessary.

As for me, I am not convinced yet, that the fully transparent way to support
PREFIX/lib/modules/ is the best way forward. I think it might be better to
first only make script/depmod.sh fit for a kmod prefix and require an adjusted
INSTALL_MOD_PATH for modules_install.

Which concrete distributions did you have in mind while composing the patches?

Kind regards,
Nicolas

--
epost|xmpp: nicolas@xxxxxxxxx irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb c82b 7d97 0932 55a0 ce7f
-- frykten for herren er opphav til kunnskap --

Attachment: signature.asc
Description: PGP signature

Next message: Jonathan Corbet: "Re: [PATCH] docs: consolidate core subsystems"
Previous message: Jonathan Corbet: "Re: [PATCH] docs: deprecated.rst: Update an example"
In reply to: Michal Suchánek: "Re: [PATCH v3] depmod: Handle installing modules under a prefix"
Next in thread: Michal Suchanek: "[PATCH kmod v2 1/4] man/depmod.d: Fix incorrect /usr/lib search path"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]