ktest: add 2nd parameter of run_command() to set the redirect target file
PATCH 1/3 ktest: add 2nd parameter of run_command() to set the redirect target file
From: Satoru Takeuchi <[email protected]>
If we'd like to set the redirect target file of run_command(), we should define $redirect before this function and should undef it after this function. Since it's user-unfriendly, add 2nd parameter of run_command() for this purpose.
--- tools/testing/ktest/ktest.pl | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
tools/testing/ktest/ktest.pl +3 -8
@@ -149,7 +149,6 @@ my $bisect_ret_abort; my $bisect_ret_default; my $in_patchcheck = 0; my $run_test; -my $redirect; my $buildlog; my $testlog; my $dmesg;
@@ -1529,7 +1528,7 @@ sub fail { } sub run_command { - my ($command) = @_; + my ($command, $redirect) = @_; my $dolog = 0; my $dord = 0; my $pid;
@@ -2265,9 +2264,7 @@ sub build { # Run old config regardless, to enforce min configurations make_oldconfig; - $redirect = "$buildlog"; - my $build_ret = run_command "$make $build_options"; - undef $redirect; + my $build_ret = run_command "$make $build_options", $buildlog; if (defined($post_build)) { # Because a post build may change the kernel version
@@ -2360,9 +2357,7 @@ sub child_run_test { $poweroff_on_error = 0; $die_on_failure = 1; - $redirect = "$testlog"; - run_command $run_test or $failed = 1; - undef $redirect; + run_command $run_test, $testlog or $failed = 1; exit $failed; }
Signature · 1 line
1.9.0
PATCH 2/3 ktest: Some cleanup for improving readability
From: Satoru Takeuchi <[email protected]>
Some cleanup for improving readability as follows.
- Initialize $ktest_config at its definition.
- Put parentheses around the `config-file' argument in the usage message because it's a optional one.
- Rename get_ktest_config{,s} to more descriptive get_mandatory_config{,s}.
--- tools/testing/ktest/ktest.pl | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-)
tools/testing/ktest/ktest.pl +20 -22
@@ -72,7 +72,7 @@ my %default = ( "IGNORE_UNUSED" => 0, ); -my $ktest_config; +my $ktest_config = "ktest.conf"; my $version; my $have_version = 0; my $machine;
@@ -521,7 +521,7 @@ sub read_ync { return read_prompt 1, $prompt; } -sub get_ktest_config { +sub get_mandatory_config { my ($config) = @_; my $ans;
@@ -552,29 +552,29 @@ sub get_ktest_config { } } -sub get_ktest_configs { - get_ktest_config("MACHINE"); - get_ktest_config("BUILD_DIR"); - get_ktest_config("OUTPUT_DIR"); +sub get_mandatory_configs { + get_mandatory_config("MACHINE"); + get_mandatory_config("BUILD_DIR"); + get_mandatory_config("OUTPUT_DIR"); if ($newconfig) { - get_ktest_config("BUILD_OPTIONS"); + get_mandatory_config("BUILD_OPTIONS"); } # options required for other than just building a kernel if (!$buildonly) { - get_ktest_config("POWER_CYCLE"); - get_ktest_config("CONSOLE"); + get_mandatory_config("POWER_CYCLE"); + get_mandatory_config("CONSOLE"); } # options required for install and more if ($buildonly != 1) { - get_ktest_config("SSH_USER"); - get_ktest_config("BUILD_TARGET"); - get_ktest_config("TARGET_IMAGE"); + get_mandatory_config("SSH_USER"); + get_mandatory_config("BUILD_TARGET"); + get_mandatory_config("TARGET_IMAGE"); } - get_ktest_config("LOCALVERSION"); + get_mandatory_config("LOCALVERSION"); return if ($buildonly);
@@ -582,7 +582,7 @@ sub get_ktest_configs { if (!defined($rtype)) { if (!defined($opt{"GRUB_MENU"})) { - get_ktest_config("REBOOT_TYPE"); + get_mandatory_config("REBOOT_TYPE"); $rtype = $entered_configs{"REBOOT_TYPE"}; } else { $rtype = "grub";
@@ -590,16 +590,16 @@ sub get_ktest_configs { } if ($rtype eq "grub") { - get_ktest_config("GRUB_MENU"); + get_mandatory_config("GRUB_MENU"); } if ($rtype eq "grub2") { - get_ktest_config("GRUB_MENU"); - get_ktest_config("GRUB_FILE"); + get_mandatory_config("GRUB_MENU"); + get_mandatory_config("GRUB_FILE"); } if ($rtype eq "syslinux") { - get_ktest_config("SYSLINUX_LABEL"); + get_mandatory_config("SYSLINUX_LABEL"); } }
@@ -1089,7 +1089,7 @@ sub read_config { $test_case = __read_config $config, \$test_num; # make sure we have all mandatory configs - get_ktest_configs; + get_mandatory_configs; # was a test specified? if (!$test_case) {
@@ -3858,7 +3858,7 @@ sub make_warnings_file { success $i; } -$#ARGV < 1 or die "ktest.pl version: $VERSION\n usage: ktest.pl config-file\n"; +$#ARGV < 1 or die "ktest.pl version: $VERSION\n usage: ktest.pl [config-file]\n"; if ($#ARGV == 0) { $ktest_config = $ARGV[0];
@@ -3868,8 +3868,6 @@ if ($#ARGV == 0) { exit 0; } } -} else { - $ktest_config = "ktest.conf"; } if (! -f $ktest_config) {
Signature · 1 line
1.9.0
PATCH 3/3 ktest: remove the misleading $buildonly and introduce $laststep.
From: Satoru Takeuchi <[email protected]>
Each test of ktest consists of the following steps.
build -> install -> boot -> run user defined tests.
$buildonly means not whether the test is build onlyor not. Actually this variable mean the last step of the test as follows.
0: boot or more
1: build
2: install
AS you can see, these are random numeric literals. In addition, there is no explanation about them.
To improve readability, introduce $laststep instead of $buildonly. This variable means the last step of the test as follows.
STEP_BUILD (=0): build
STEP_INSTALL (=1): install
STEP_BOOT_OR_MORE (=2): boot or more
--- tools/testing/ktest/ktest.pl | 70 +++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 37 deletions(-)
tools/testing/ktest/ktest.pl +33 -37
@@ -196,9 +196,14 @@ my $patchcheck_type; my $patchcheck_start; my $patchcheck_end; -# set when a test is something other that just building or install -# which would require more options. -my $buildonly = 1; +# Each test of ktest consists of the following steps. +use constant { + STEP_BUILD => 0, + STEP_INSTALL => 1, + STEP_BOOT_OR_MORE => 2, +}; +# The last step of test. +my $laststep = STEP_BUILD; # tell build not to worry about warnings, even when WARNINGS_FILE is set my $warnings_ok = 0;
@@ -553,30 +558,27 @@ sub get_mandatory_config { } sub get_mandatory_configs { + # Options required for build step. get_mandatory_config("MACHINE"); get_mandatory_config("BUILD_DIR"); get_mandatory_config("OUTPUT_DIR"); + get_mandatory_config("LOCALVERSION"); if ($newconfig) { get_mandatory_config("BUILD_OPTIONS"); } + return if ($laststep == STEP_BUILD); - # options required for other than just building a kernel - if (!$buildonly) { - get_mandatory_config("POWER_CYCLE"); - get_mandatory_config("CONSOLE"); - } + # Options required for install step. + get_mandatory_config("SSH_USER"); + get_mandatory_config("BUILD_TARGET"); + get_mandatory_config("TARGET_IMAGE"); - # options required for install and more - if ($buildonly != 1) { - get_mandatory_config("SSH_USER"); - get_mandatory_config("BUILD_TARGET"); - get_mandatory_config("TARGET_IMAGE"); - } - - get_mandatory_config("LOCALVERSION"); + return if ($laststep == STEP_INSTALL); - return if ($buildonly); + # Options required for boot step. + get_mandatory_config("POWER_CYCLE"); + get_mandatory_config("CONSOLE"); my $rtype = $opt{"REBOOT_TYPE"};
@@ -649,26 +651,20 @@ sub set_value { my $prvalue = process_variables($rvalue); - if ($buildonly && $lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { - # Note if a test is something other than build, then we - # will need other manditory options. - if ($prvalue ne "install") { - # for bisect, we need to check BISECT_TYPE - if ($prvalue ne "bisect") { - $buildonly = 0; + if ($laststep <= STEP_INSTALL) { + if ($lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { + # Note if a test is something other than build, then we + # will need other manditory options. + if ($prvalue eq "install") { + # install still limits some manditory options. + $laststep = STEP_INSTALL; + } elsif ($prvalue ne "bisect") { + # for bisect, we need to check BISECT_TYPE + $laststep = STEP_BUILD; } - } else { - # install still limits some manditory options. - $buildonly = 2; - } - } - - if ($buildonly && $lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && $prvalue ne "build") { - if ($prvalue ne "install") { - $buildonly = 0; - } else { - # install still limits some manditory options. - $buildonly = 2; + } elsif ($lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && + $prvalue ne "build") { + $laststep = ($prvalue eq "install") ? STEP_INSTALL : STEP_BUILD; } }
@@ -4045,7 +4041,7 @@ for (my $i = 1; $i <= $opt{"NUM_TESTS"}; $i++) { $dmesg = "$tmpdir/dmesg-$machine"; $output_config = "$outputdir/.config"; - if (!$buildonly) { + if ($laststep >= STEP_BOOT_OR_MORE) { $target = "$ssh_user\@$machine"; if ($reboot_type eq "grub") { dodie "GRUB_MENU not defined" if (!defined($grub_menu));
Signature · 1 line
1.9.0
Steven Rostedt
maintainerRe: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
Quoted 16 lines · On Sun, 09 Mar 2014 23:36:49 +0900 Satoru Takeuchi <[email protected]>…
From: Satoru Takeuchi <[email protected]>
Each test of ktest consists of the following steps.
build -> install -> boot -> run user defined tests.
$buildonly means not whether the test is build onlyor not. Actually this variable mean the last step of the test as follows.
0: boot or more
1: build
2: installAS you can see, these are random numeric literals. In addition, there is no explanation about them.
Quoted 6 lines
To improve readability, introduce $laststep instead of $buildonly. This variable means the last step of the test as follows.
STEP_BUILD(=0): build
STEP_INSTALL(=1): install
STEP_BOOT_OR_MORE(=2): boot or more
Nice clean up. But there's some bugs in this patch.
tools/testing/ktest/ktest.pl sub set_value { · line 658
Quoted 11 lines
tools/testing/ktest/ktest.pl +0
-7@@ -649,26 +651,20 @@ sub set_value { my $prvalue = process_variables($rvalue); - if ($buildonly && $lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { - # Note if a test is something other than build, then we - # will need other manditory options. - if ($prvalue ne "install") { - # for bisect, we need to check BISECT_TYPE - if ($prvalue ne "bisect") { - $buildonly = 0;
When prvalue ne "bisect" we set it to boot or more.
tools/testing/ktest/ktest.pl sub set_value { · line 663
Quoted 10 lines
tools/testing/ktest/ktest.pl +10
-0+ if ($laststep <= STEP_INSTALL) { + if ($lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { + # Note if a test is something other than build, then we + # will need other manditory options. + if ($prvalue eq "install") { + # install still limits some manditory options. + $laststep = STEP_INSTALL; + } elsif ($prvalue ne "bisect") { + # for bisect, we need to check BISECT_TYPE + $laststep = STEP_BUILD;
Here you set it back to build.
Quoted 6 lines
tools/testing/ktest/ktest.pl +0
-5} - } else { - # install still limits some manditory options. - $buildonly = 2; - } - }
tools/testing/ktest/ktest.pl sub set_value { · line 667
Quoted 10 lines
tools/testing/ktest/ktest.pl +3
-7- - if ($buildonly && $lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && $prvalue ne "build") { - if ($prvalue ne "install") { - $buildonly = 0; - } else { - # install still limits some manditory options. - $buildonly = 2; + } elsif ($lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && + $prvalue ne "build") { + $laststep = ($prvalue eq "install") ? STEP_INSTALL : STEP_BUILD;
Here too.
In fact, with this patch, we never set to boot or more.
Also, you can make it even cleaner, by having the outer if condition be:
if ($laststep <= STEP_INSTALL && $prvalue ne "build")
And remove the prvalue check from the inner conditions.
Just send a fix of this patch, I have already pulled in the other two. I just need to test them for a bit before I push them to my kernel.org repo. I don't actually have a test suite for ktest. My testing is that I use ktest on a daily basis, and I just use the latest devel ktest for my daily activities. If something breaks, I usually notice, unless it's affects something I haven't done recently (like a bisect).
Thanks,
-- Steve
Quoted 12 lines
tools/testing/ktest/ktest.pl +1
-1} }@@ -4045,7 +4041,7 @@ for (my $i = 1; $i <= $opt{"NUM_TESTS"}; $i++) { $dmesg = "$tmpdir/dmesg-$machine"; $output_config = "$outputdir/.config"; - if (!$buildonly) { + if ($laststep >= STEP_BOOT_OR_MORE) { $target = "$ssh_user\@$machine"; if ($reboot_type eq "grub") { dodie "GRUB_MENU not defined" if (!defined($grub_menu));
Re: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
At Mon, 10 Mar 2014 12:13:00 -0400,
Quoted 76 lines · Steven Rostedt
Quoted 16 lines · On Sun, 09 Mar 2014 23:36:49 +0900 Satoru Takeuchi <[email protected]>…
From: Satoru Takeuchi <[email protected]>
Each test of ktest consists of the following steps.
build -> install -> boot -> run user defined tests.
$buildonly means not whether the test is build onlyor not. Actually this variable mean the last step of the test as follows.
0: boot or more
1: build
2: installAS you can see, these are random numeric literals. In addition, there is no explanation about them.
Quoted 6 lines
To improve readability, introduce $laststep instead of $buildonly. This variable means the last step of the test as follows.
STEP_BUILD(=0): build
STEP_INSTALL(=1): install
STEP_BOOT_OR_MORE(=2): boot or moreNice clean up. But there's some bugs in this patch.
tools/testing/ktest/ktest.pl sub set_value { · line 658
Quoted 11 lines
tools/testing/ktest/ktest.pl +0
-7@@ -649,26 +651,20 @@ sub set_value { my $prvalue = process_variables($rvalue); - if ($buildonly && $lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { - # Note if a test is something other than build, then we - # will need other manditory options. - if ($prvalue ne "install") { - # for bisect, we need to check BISECT_TYPE - if ($prvalue ne "bisect") { - $buildonly = 0;When prvalue ne "bisect" we set it to boot or more.
tools/testing/ktest/ktest.pl sub set_value { · line 663
Quoted 10 lines
tools/testing/ktest/ktest.pl +10
-0+ if ($laststep <= STEP_INSTALL) { + if ($lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { + # Note if a test is something other than build, then we + # will need other manditory options. + if ($prvalue eq "install") { + # install still limits some manditory options. + $laststep = STEP_INSTALL; + } elsif ($prvalue ne "bisect") { + # for bisect, we need to check BISECT_TYPE + $laststep = STEP_BUILD;Here you set it back to build.
Quoted 6 lines
tools/testing/ktest/ktest.pl +0
-5} - } else { - # install still limits some manditory options. - $buildonly = 2; - } - }tools/testing/ktest/ktest.pl sub set_value { · line 667
Quoted 10 lines
tools/testing/ktest/ktest.pl +3
-7- - if ($buildonly && $lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && $prvalue ne "build") { - if ($prvalue ne "install") { - $buildonly = 0; - } else { - # install still limits some manditory options. - $buildonly = 2; + } elsif ($lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && + $prvalue ne "build") { + $laststep = ($prvalue eq "install") ? STEP_INSTALL : STEP_BUILD;Here too.
Quoted 1 line
In fact, with this patch, we never set to boot or more.
Sorry, my code review and test are insufficient.
Quoted 6 lines
Also, you can make it even cleaner, by having the outer if condition be:
if ($laststep <= STEP_INSTALL && $prvalue ne "build")And remove the prvalue check from the inner conditions.
Thank you fo your comment.
Quoted 2 lines
Just send a fix of this patch, I have already pulled in the other two.
Thanks, I'll do.
Quoted 5 lines
I just need to test them for a bit before I push them to my kernel.org repo. I don't actually have a test suite for ktest. My testing is that I use ktest on a daily basis, and I just use the latest devel ktest for my daily activities. If something breaks, I usually notice, unless it's affects something I haven't done recently (like a bisect).
I'll make and send my testsuite later. I considers that tools/testing/ktest/example is suitable to place it.
Thanks,
Satoru
Quoted 18 lines
Thanks,
-- Steve
Quoted 12 lines
tools/testing/ktest/ktest.pl +1
-1} }@@ -4045,7 +4041,7 @@ for (my $i = 1; $i <= $opt{"NUM_TESTS"}; $i++) { $dmesg = "$tmpdir/dmesg-$machine"; $output_config = "$outputdir/.config"; - if (!$buildonly) { + if ($laststep >= STEP_BOOT_OR_MORE) { $target = "$ssh_user\@$machine"; if ($reboot_type eq "grub") { dodie "GRUB_MENU not defined" if (!defined($grub_menu));
Re: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
At Tue, 11 Mar 2014 20:43:40 +0900,
Quoted 97 lines · Satoru Takeuchi
At Mon, 10 Mar 2014 12:13:00 -0400,
Quoted 76 lines · Steven Rostedt
Quoted 16 lines · On Sun, 09 Mar 2014 23:36:49 +0900 Satoru Takeuchi <[email protected]>…
From: Satoru Takeuchi <[email protected]>
Each test of ktest consists of the following steps.
build -> install -> boot -> run user defined tests.
$buildonly means not whether the test is build onlyor not. Actually this variable mean the last step of the test as follows.
0: boot or more
1: build
2: installAS you can see, these are random numeric literals. In addition, there is no explanation about them.
Quoted 6 lines
To improve readability, introduce $laststep instead of $buildonly. This variable means the last step of the test as follows.
STEP_BUILD(=0): build
STEP_INSTALL(=1): install
STEP_BOOT_OR_MORE(=2): boot or moreNice clean up. But there's some bugs in this patch.
tools/testing/ktest/ktest.pl sub set_value { · line 658
Quoted 11 lines
tools/testing/ktest/ktest.pl +0
-7@@ -649,26 +651,20 @@ sub set_value { my $prvalue = process_variables($rvalue); - if ($buildonly && $lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { - # Note if a test is something other than build, then we - # will need other manditory options. - if ($prvalue ne "install") { - # for bisect, we need to check BISECT_TYPE - if ($prvalue ne "bisect") { - $buildonly = 0;When prvalue ne "bisect" we set it to boot or more.
tools/testing/ktest/ktest.pl sub set_value { · line 663
Quoted 10 lines
tools/testing/ktest/ktest.pl +10
-0+ if ($laststep <= STEP_INSTALL) { + if ($lvalue =~ /^TEST_TYPE(\[.*\])?$/ && $prvalue ne "build") { + # Note if a test is something other than build, then we + # will need other manditory options. + if ($prvalue eq "install") { + # install still limits some manditory options. + $laststep = STEP_INSTALL; + } elsif ($prvalue ne "bisect") { + # for bisect, we need to check BISECT_TYPE + $laststep = STEP_BUILD;Here you set it back to build.
Quoted 6 lines
tools/testing/ktest/ktest.pl +0
-5} - } else { - # install still limits some manditory options. - $buildonly = 2; - } - }tools/testing/ktest/ktest.pl sub set_value { · line 667
Quoted 10 lines
tools/testing/ktest/ktest.pl +3
-7- - if ($buildonly && $lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && $prvalue ne "build") { - if ($prvalue ne "install") { - $buildonly = 0; - } else { - # install still limits some manditory options. - $buildonly = 2; + } elsif ($lvalue =~ /^BISECT_TYPE(\[.*\])?$/ && + $prvalue ne "build") { + $laststep = ($prvalue eq "install") ? STEP_INSTALL : STEP_BUILD;Here too.
Quoted 1 line
In fact, with this patch, we never set to boot or more.
Sorry, my code review and test are insufficient.
Quoted 6 lines
Also, you can make it even cleaner, by having the outer if condition be:
if ($laststep <= STEP_INSTALL && $prvalue ne "build")And remove the prvalue check from the inner conditions.
Thank you fo your comment.
Quoted 2 lines
Just send a fix of this patch, I have already pulled in the other two.
Thanks, I'll do.
Quoted 8 lines
Quoted 5 lines
I just need to test them for a bit before I push them to my kernel.org repo. I don't actually have a test suite for ktest. My testing is that I use ktest on a daily basis, and I just use the latest devel ktest for my daily activities. If something breaks, I usually notice, unless it's affects something I haven't done recently (like a bisect).
I'll make and send my testsuite later. I considers that
tools/testing/ktest/exampleis suitable to place it.
Sorry, actually tools/testing/ktest/tests.
.../examples/ is for examples for configs file as written in examples/README.
Thanks,
Satoru
Steven Rostedt
maintainerRe: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
Quoted 11 lines · On Tue, 11 Mar 2014 20:50:53 +0900 Satoru Takeuchi <[email protected]>…
Quoted 8 lines
Quoted 5 lines
I just need to test them for a bit before I push them to my kernel.org repo. I don't actually have a test suite for ktest. My testing is that I use ktest on a daily basis, and I just use the latest devel ktest for my daily activities. If something breaks, I usually notice, unless it's affects something I haven't done recently (like a bisect).
I'll make and send my testsuite later. I considers that
tools/testing/ktest/exampleis suitable to place it.Sorry, actually
tools/testing/ktest/tests.
.../examples/ is for examples for configs file as written in examples/README.
If the code looks clean, I have no problems with adding it.
Perhaps call the directory "selftests" that way it wont be confusing to what is there. Especially if you add a README that states that the tests in that directory are for the purpose of testing ktest.pl itself.
-- Steve