Re: [PATCH RESEND v4 1/3] kselftests: lib.mk: Add TEST_GEN_MODS_DIR variable

[Marcos Paulo de Souza <mpdesouza@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 8568 bytes --]

On Wed, 2024-01-03 at 15:09 -0700, Shuah Khan wrote:
> On 1/2/24 15:31, Joe Lawrence wrote:
> > On Wed, Dec 20, 2023 at 01:53:12PM -0300, Marcos Paulo de Souza
> > wrote:
> > > Add TEST_GEN_MODS_DIR variable for kselftests. It can point to
> > > a directory containing kernel modules that will be used by
> > > selftest scripts.
> > > 
> > > The modules are built as external modules for the running kernel.
> > > As a result they are always binary compatible and the same tests
> > > can be used for older or newer kernels.
> > > 
> > > The build requires "kernel-devel" package to be installed.
> > > For example, in the upstream sources, the rpm devel package
> > > is produced by "make rpm-pkg"
> > > 
> > > The modules can be built independently by
> > > 
> > >    make -C tools/testing/selftests/livepatch/
> > > 
> > > or they will be automatically built before running the tests via
> > > 
> > >    make -C tools/testing/selftests/livepatch/ run_tests
> > > 
> > > Note that they are _not_ built when running the standalone
> > > tests by calling, for example, ./test-state.sh.
> > > 
> > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > > ---
> > >   Documentation/dev-tools/kselftest.rst |  4 ++++
> > >   tools/testing/selftests/lib.mk        | 20 +++++++++++++++-----
> > >   2 files changed, 19 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/dev-tools/kselftest.rst
> > > b/Documentation/dev-tools/kselftest.rst
> > > index ab376b316c36..7f3582a67318 100644
> > > --- a/Documentation/dev-tools/kselftest.rst
> > > +++ b/Documentation/dev-tools/kselftest.rst
> > > @@ -245,6 +245,10 @@ Contributing new tests (details)
> > >      TEST_PROGS, TEST_GEN_PROGS mean it is the executable tested
> > > by
> > >      default.
> > >   
> > > +   TEST_GEN_MODS_DIR should be used by tests that require
> > > modules to be built
> > > +   before the test starts. The variable will contain the name of
> > > the directory
> > > +   containing the modules.
> > > +
> > >      TEST_CUSTOM_PROGS should be used by tests that require
> > > custom build
> > >      rules and prevent common build rule use.
> > >   
> > > diff --git a/tools/testing/selftests/lib.mk
> > > b/tools/testing/selftests/lib.mk
> > > index 118e0964bda9..6c7c5a0112cf 100644
> > > --- a/tools/testing/selftests/lib.mk
> > > +++ b/tools/testing/selftests/lib.mk
> > > @@ -70,12 +70,15 @@ KHDR_INCLUDES := -isystem $(KHDR_DIR)
> > >   # TEST_PROGS are for test shell scripts.
> > >   # TEST_CUSTOM_PROGS and TEST_PROGS will be run by common
> > > run_tests
> > >   # and install targets. Common clean doesn't touch them.
> > > +# TEST_GEN_MODS_DIR is used to specify a directory with modules
> > > to be built
> > > +# before the test executes. These modules are cleaned on the
> > > clean target as well.
> > >   TEST_GEN_PROGS := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_PROGS))
> > >   TEST_GEN_PROGS_EXTENDED := $(patsubst
> > > %,$(OUTPUT)/%,$(TEST_GEN_PROGS_EXTENDED))
> > >   TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
> > > +TEST_GEN_MODS_DIR := $(patsubst
> > > %,$(OUTPUT)/%,$(TEST_GEN_MODS_DIR))
> > >   
> > >   all: kernel_header_files $(TEST_GEN_PROGS)
> > > $(TEST_GEN_PROGS_EXTENDED) \
> > > -     $(TEST_GEN_FILES)
> > > +     $(TEST_GEN_FILES) $(if $(TEST_GEN_MODS_DIR),gen_mods_dir)
> > >   
> > >   kernel_header_files:
> > >   	@ls $(KHDR_DIR)/linux/*.h >/dev/null
> > > 2>/dev/null;                      \
> > > @@ -105,8 +108,8 @@ endef
> > >   
> > >   run_tests: all
> > >   ifdef building_out_of_srctree
> > > -	@if [
> > > "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)" != "X" ];
> > > then \
> > > -		rsync -aq --copy-unsafe-links $(TEST_PROGS)
> > > $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT); \
> > > +	@if [
> > > "X$(TEST_PROGS)$(TEST_PROGS_EXTENDED)$(TEST_FILES)$(TEST_GEN_MODS
> > > _DIR)" != "X" ]; then \
> > > +		rsync -aq --copy-unsafe-links $(TEST_PROGS)
> > > $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(TEST_GEN_MODS_DIR)
> > > $(OUTPUT); \
> > >   	fi
> > >   	@if [ "X$(TEST_PROGS)" != "X" ]; then \
> > >   		$(call RUN_TESTS, $(TEST_GEN_PROGS)
> > > $(TEST_CUSTOM_PROGS) \
> > > @@ -118,6 +121,12 @@ else
> > >   	@$(call RUN_TESTS, $(TEST_GEN_PROGS)
> > > $(TEST_CUSTOM_PROGS) $(TEST_PROGS))
> > >   endif
> > >   
> > > +gen_mods_dir:
> > > +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR)
> > > +
> > > +clean_mods_dir:
> > > +	$(Q)$(MAKE) -C $(TEST_GEN_MODS_DIR) clean
> > > +
> > >   define INSTALL_SINGLE_RULE
> > >   	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH))
> > >   	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links
> > > $(INSTALL_LIST) $(INSTALL_PATH)/)
> > > @@ -131,6 +140,7 @@ define INSTALL_RULE
> > >   	$(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS))
> > > $(INSTALL_SINGLE_RULE)
> > >   	$(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED))
> > > $(INSTALL_SINGLE_RULE)
> > >   	$(eval INSTALL_LIST = $(TEST_GEN_FILES))
> > > $(INSTALL_SINGLE_RULE)
> > > +	$(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR))
> > > $(INSTALL_SINGLE_RULE)
> > 
> > Hi Marcos,
> > 
> > Sorry for the late reply on this, but I'm reviewing this version by
> > trying to retrofit it into our selftest packaging (pre-build the
> > test
> > module .ko's and stash those into an rpm rather than building on
> > the
> > test host).
> > 
> > Since $TEST_GEN_MODS_DIR is treated as a directory, I found that
> > the
> > selftest install target copies a bunch of intermediate object and
> > kbuild
> > files:
> > 
> >    $ mkdir /tmp/test-install
> >    $ make KDIR=$(pwd) INSTALL_PATH=/tmp/test-install
> > TARGETS=livepatch \
> >         -C tools/testing/selftests/ install
> > 
> >    [ ... builds livepatch selftests ... ]
> > 
> > the rsync in question:
> > 
> >    rsync -a --copy-unsafe-links
> > /home/jolawren/src/kernel/tools/testing/selftests/livepatch/test_mo
> > dules /tmp/test-install/livepatch/
> >    ...
> > 
> > and then looking at the destination:
> > 
> >    $ tree -a /tmp/test-install/
> >    /tmp/test-install/
> >    ├── kselftest
> >    │   ├── module.sh
> >    │   ├── prefix.pl
> >    │   └── runner.sh
> >    ├── kselftest-list.txt
> >    ├── livepatch
> >    │   ├── config
> >    │   ├── functions.sh
> >    │   ├── settings
> >    │   ├── test-callbacks.sh
> >    │   ├── test-ftrace.sh
> >    │   ├── test_klp-call_getpid
> >    │   ├── test-livepatch.sh
> >    │   ├── test_modules
> >    │   │   ├── Makefile
> >    │   │   ├── modules.order
> >    │   │   ├── .modules.order.cmd
> >    │   │   ├── Module.symvers
> >    │   │   ├── .Module.symvers.cmd
> >    │   │   ├── test_klp_atomic_replace.c
> >    │   │   ├── test_klp_atomic_replace.ko
> >    │   │   ├── .test_klp_atomic_replace.ko.cmd
> >    │   │   ├── test_klp_atomic_replace.mod
> >    │   │   ├── test_klp_atomic_replace.mod.c
> >    │   │   ├── .test_klp_atomic_replace.mod.cmd
> >    │   │   ├── test_klp_atomic_replace.mod.o
> >    │   │   ├── .test_klp_atomic_replace.mod.o.cmd
> >    │   │   ├── test_klp_atomic_replace.o
> >    │   │   ├── .test_klp_atomic_replace.o.cmd
> >    ...
> > 
> > On the other hand, variables like $TEST_GEN_FILES specify
> > individual
> > files, so only final binaries like test_klp-call_getpid (and not
> > test_klp-call_getpid.c) are copied to $INSTALL_PATH.

Hi Joe,

thanks for catching this issue. I crafted the attached patch and it
fixes the issue for me, copying only the resulting .ko objects as
expected. Can you please check if this fixes the issue for you?

> 
> 
> Thank you Joe for finding this problem.
> 
> Copying source files and object files doesn't sound right. This isn't
> how the ksleftest installs work. Let's fix this.

Hi Shuah,

what do you think about the proposed solution? Could you please amend
the fix into the first patch if you think it's the right approach?

Thanks in advance!
  Marcos

> 
> thanks,
> --Shuah
> 


[-- Attachment #2: Type: text/x-patch, Size: 1176 bytes --]

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 6c7c5a0112cf..d77dda59f6a2 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -132,6 +132,11 @@ define INSTALL_SINGLE_RULE
 	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST) $(INSTALL_PATH)/)
 endef
 
+define INSTALL_MODS_RULE
+	$(if $(INSTALL_LIST),@mkdir -p $(INSTALL_PATH)/$(INSTALL_LIST))
+	$(if $(INSTALL_LIST),rsync -a --copy-unsafe-links $(INSTALL_LIST)/*.ko $(INSTALL_PATH)/$(INSTALL_LIST))
+endef
+
 define INSTALL_RULE
 	$(eval INSTALL_LIST = $(TEST_PROGS)) $(INSTALL_SINGLE_RULE)
 	$(eval INSTALL_LIST = $(TEST_PROGS_EXTENDED)) $(INSTALL_SINGLE_RULE)
@@ -140,7 +145,7 @@ define INSTALL_RULE
 	$(eval INSTALL_LIST = $(TEST_CUSTOM_PROGS)) $(INSTALL_SINGLE_RULE)
 	$(eval INSTALL_LIST = $(TEST_GEN_PROGS_EXTENDED)) $(INSTALL_SINGLE_RULE)
 	$(eval INSTALL_LIST = $(TEST_GEN_FILES)) $(INSTALL_SINGLE_RULE)
-	$(eval INSTALL_LIST = $(TEST_GEN_MODS_DIR)) $(INSTALL_SINGLE_RULE)
+	$(eval INSTALL_LIST = $(notdir $(TEST_GEN_MODS_DIR))) $(INSTALL_MODS_RULE)
 	$(eval INSTALL_LIST = $(wildcard config settings)) $(INSTALL_SINGLE_RULE)
 endef