On Sat 2024-12-21 01:05:35, Siddharth Menon wrote:

This patch adds a script to validate that the current kernel configuration satisfies the requirements for selftests. The script compares the current kernel configs against the required selftest configs.

A config mismatch exits with error value 1 while matching configs or missing config files exit with value 0.In order to get debug output, set the

^ missing space before the next sentence

environment variable LOCALMODCONFIG_DEBUG=1.

I would prefer to print the missing dependencies by default. Otherwise, it is far from clear why the test was skipped.

Also LOCALMODCONFIG_DEBUG looks like a pretty non-standard way to get debug output. People will have troubles to discover this way and memorize the long name.

diff --git a/tools/testing/selftests/mktest.pl b/tools/testing/selftests/mktest.pl new file mode 100755 index 000000000000..60462f323bde --- /dev/null +++ b/tools/testing/selftests/mktest.pl

I would give it a more meaningful name.

For example, using the same name as for the makefile target just with .pl suffix. I mean "check_config_deps.pl".

That said, I have just realized that that there already was "kselftest_deps.sh". It seems to check missing compile dependencies by scanning various variants of LDLIBS variables.

It seems that "kselftest_deps.sh" should be called separately. I mean that it is not integrated with the make files.

It is a bit strange to handle two types of dependencies differently. It would be nice somehow consolidate the two check scripts so that they behave similar way and use a similar name. I personally like the integration with the Makefile.

@@ -0,0 +1,138 @@ +#!/usr/bin/env perl +# SPDX-License-Identifier: GPL-2.0 +use warnings; +use strict; +use Getopt::Long; +use File::Spec;

+# set the environment variable LOCALMODCONFIG_DEBUG to get +# debug output. +my $debugprint = 0; +$debugprint = 1 if (defined($ENV{LOCALMODCONFIG_DEBUG}));

+sub dprint {

• return if (!$debugprint);
• print STDERR @_;

+}

+my $uname = `uname -r`; +chomp $uname;

+my @searchconfigs = (

• {

•    "file" => ".config",
  

•    "exec" => "cat",
  

Nit: I would prefer to use a tab instead of 8 spaces. At least, the 8 spaces are considered a bad formatting in the kernel .c sources.

• },
• {

•    "file" => "/proc/config.gz",
  

•    "exec" => "zcat",
  

• },
• {

•    "file" => "/boot/config-$uname",
  

•    "exec" => "cat",
  

• },
• {

•    "file" => "/boot/vmlinuz-$uname",
  

•    "exec" => "scripts/extract-ikconfig",
  

•    "test" => "scripts/extract-ikconfig",
  

• },
• {

•    "file" => "vmlinux",
  

•    "exec" => "scripts/extract-ikconfig",
  

•    "test" => "scripts/extract-ikconfig",
  

• },
• {

•    "file" => "/lib/modules/$uname/kernel/kernel/configs.ko",
  

•    "exec" => "scripts/extract-ikconfig",
  

•    "test" => "scripts/extract-ikconfig",
  

• },
• {

•    "file" => "/lib/modules/$uname/build/.config",
  

•    "exec" => "cat",
  

• },
• {

•    "file" => "kernel/configs.ko",
  

•    "exec" => "scripts/extract-ikconfig",
  

•    "test" => "scripts/extract-ikconfig",
  

• },
• {

•    "file" => "kernel/configs.o",
  

•    "exec" => "scripts/extract-ikconfig",
  

•    "test" => "scripts/extract-ikconfig",
  

• },

+);

+sub read_config {

• foreach my $conf (@searchconfigs) {
• my $file = $conf->{"file"};
• next if (! -f "$file");

Nit: There are 4 spaces on the above empty line. "git am" complains about it.

I guess that also ./scripts/checkpatch.pl would complain about it.

• if (defined($conf->{"test"})) {

•    `$conf->{"test"} $conf->{"file"} 2>/dev/null`;
  

•    next if ($?);
  

• }

same here

• my $exec = $conf->{"exec"};

and here

• dprint "Kernel config: '$file'\n";

and here.

• open(my $infile, '-|', "$exec $file") || die "Failed to run $exec $file";
• my @x = <$infile>;
• close $infile;
• return @x;
• }
• dprint "Unable to find kernel config file, skipping check\n";
• exit 0;

+}

+# Check if selftest path is provided +die "Usage: $0 <selftest_path>\n" unless @ARGV == 1;

+my $file_path = $ARGV[0];

+my @config_file = read_config();

+my %kern_configs; +my $valid = "A-Za-z_0-9"; +foreach my $line (@config_file) {

• chomp $line;
• next if $line =~ /^\s*$/ || $line =~ /^#/;

another spaces on empty line

• if ($line =~ /^(CONFIG_\w+)=(.+)$/) {

•    $kern_configs{$1} = $2;
  

• }

+}

+my %test_configs; +# Continue as normal if /config file does not exist

trailing space on the above line.

+open(my $fh, '<', $file_path) or exit 0;

+while (my $line = <$fh>) {

• chomp $line;
• next if $line =~ /^\s*$/ || $line =~ /^#/;

spaces on empty line

• if ($line =~ /^(CONFIG_\w+)=(.+)$/) {

•    $test_configs{$1} = $2;
  

• }

+} +close $fh;

Best Regards, Petr

On Sat 2024-12-21 01:05:36, Siddharth Menon wrote:

Currently, kselftests does not have a generalised mechanism to skip compilation and run tests when required kernel configuration flags are missing.

This patch introduces a check to validate the presence of required config flags specified in the selftest config files. In case scripts/config or the current kernel config is not found, this check is skipped.

In order to view the missing config options required to compile the test, set the environment variable LOCALMODCONFIG_DEBUG=1.

As I wrote in the review for the 1st patch, I would prefer to print the missing config options by default. The LOCALMODCONFIG_DEBUG variable is pretty non-standard and hard to memorize thing.

--- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -97,7 +97,14 @@ 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)) -all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) \ +TEST_DIR := $(shell pwd)

+check_config_deps:

• @$(selfdir)/mktest.pl $(TEST_DIR)/config || \
• { echo "Skipping test: $(notdir $(TEST_DIR))"; exit 1; }

I would write a more meaningful message, e.g.

{ echo "Skipping test because of missing kernel features: $(notdir $(TEST_DIR))"; exit 1; }

+all: check_config_deps $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) \ $(if $(TEST_GEN_MODS_DIR),gen_mods_dir) define RUN_TESTS @@ -228,4 +235,4 @@ $(OUTPUT)/%:%.S $(LINK.S) $^ $(LDLIBS) -o $@ endif

Otherwise, it seems to work well for the livepatching selftests.

I guess that it might prevent running some selftests because of too strict or outdated information in some tools/testing/selftests/<project>/config files. So that it might cause regressions.

But I think that this is the right way to go. I am just not sure whether we should wait for complains from linux-next. Or if we should be more proactive in fixing the various <project>/config files.

Best Regards, Petr