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

[Petr Mladek <pmladek@suse.com>]

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