[PATCH v2 0/2] update kselftest framework to check for required configs

[PATCH v2 0/2] update kselftest framework to check for required configs

[Siddharth Menon]

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

This patch series adresses this issue by checking whether all required configs
from selftest/<test>/config are enabled in the current kernel

Siddharth Menon (2):
  selftests: Introduce script to validate required configs
  selftests/lib.mk: Introduce check to validate required configs

 tools/testing/selftests/lib.mk    |  11 ++-
 tools/testing/selftests/mktest.pl | 138 ++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/lib.mk
 create mode 100755 tools/testing/selftests/mktest.pl

-- 
2.39.5

[PATCH v2 1/2] selftests: Introduce script to validate required configs

[Siddharth Menon]

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
environment variable LOCALMODCONFIG_DEBUG=1.

The code for extracting the current kernel configs is adapted from
scripts/kconfig/streamline_config.pl.

Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Siddharth Menon <simeddon@gmail.com>
---
 v1->v2: created this perl script instead of checking directly in lib.mk
 tools/testing/selftests/mktest.pl | 138 ++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)
 create mode 100755 tools/testing/selftests/mktest.pl

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
@@ -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",
+    },
+    {
+        "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");
+    
+    if (defined($conf->{"test"})) {
+        `$conf->{"test"} $conf->{"file"} 2>/dev/null`;
+        next if ($?);
+    }
+    
+    my $exec = $conf->{"exec"};
+    
+    dprint "Kernel config: '$file'\n";
+    
+    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 =~ /^#/;
+    
+    if ($line =~ /^(CONFIG_\w+)=(.+)$/) {
+        $kern_configs{$1} = $2;
+    }
+}
+
+my %test_configs;
+# Continue as normal if /config file does not exist 
+open(my $fh, '<', $file_path) or exit 0;
+
+while (my $line = <$fh>) {
+    chomp $line;
+    next if $line =~ /^\s*$/ || $line =~ /^#/;
+    
+    if ($line =~ /^(CONFIG_\w+)=(.+)$/) {
+        $test_configs{$1} = $2;
+    }
+}
+close $fh;
+
+# Check if all selftest configs match kernel config
+my $all_match = 1;
+my @missing_or_mismatched;
+
+foreach my $key (keys %test_configs) {
+    if (!exists $kern_configs{$key} || $kern_configs{$key} ne $test_configs{$key}) {
+        push @missing_or_mismatched, "Required: $key=$test_configs{$key}";
+        $all_match = 0;
+    }
+}
+
+
+if ($all_match) {
+    exit 0;
+} else {
+    dprint "$_\n" for @missing_or_mismatched;
+    exit 1;
+}
-- 
2.39.5

[PATCH v2 2/2] selftests/lib.mk: Introduce check to validate required configs

[Siddharth Menon]

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.

example usage:
```
LOCALMODCONFIG_DEBUG=1 make -C livepatch/
```

Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Siddharth Menon <simeddon@gmail.com>
---
 v1->v2: moved the config checking logic to a separate perl script
 tools/testing/selftests/lib.mk | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/lib.mk

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
old mode 100644
new mode 100755
index d6edcfcb5be8..98dda6d8d702
--- 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; }
+
+
+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
 
-.PHONY: run_tests all clean install emit_tests gen_mods_dir clean_mods_dir
+.PHONY: run_tests all clean install emit_tests gen_mods_dir clean_mods_dir check_config_deps
-- 
2.39.5

Re: [PATCH v2 1/2] selftests: Introduce script to validate required configs

[Petr Mladek]

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

Re: [PATCH v2 2/2] selftests/lib.mk: Introduce check to validate required configs

[Petr Mladek]

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