All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] ktest: add 2nd parameter of run_command() to set the redirect target file
@ 2014-03-09 14:29 Satoru Takeuchi
  2014-03-09 14:32 ` [PATCH 2/3] ktest: Some cleanup for improving readability Satoru Takeuchi
  0 siblings, 1 reply; 7+ messages in thread
From: Satoru Takeuchi @ 2014-03-09 14:29 UTC (permalink / raw)
  To: LKML; +Cc: Steven Rostedt

From: Satoru Takeuchi <satoru.takeuchi@gmail.com>

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.

Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 tools/testing/ktest/ktest.pl | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/tools/testing/ktest/ktest.pl b/tools/testing/ktest/ktest.pl
index 4063156..f731ef6 100755
--- a/tools/testing/ktest/ktest.pl
+++ b/tools/testing/ktest/ktest.pl
@@ -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;
 }
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] ktest: Some cleanup for improving readability
  2014-03-09 14:29 [PATCH 1/3] ktest: add 2nd parameter of run_command() to set the redirect target file Satoru Takeuchi
@ 2014-03-09 14:32 ` Satoru Takeuchi
  2014-03-09 14:36   ` [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep Satoru Takeuchi
  0 siblings, 1 reply; 7+ messages in thread
From: Satoru Takeuchi @ 2014-03-09 14:32 UTC (permalink / raw)
  To: LKML; +Cc: Steven Rostedt

From: Satoru Takeuchi <satoru.takeuchi@gmail.com>

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}.

Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 tools/testing/ktest/ktest.pl | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/tools/testing/ktest/ktest.pl b/tools/testing/ktest/ktest.pl
index f731ef6..c34f0de 100755
--- a/tools/testing/ktest/ktest.pl
+++ b/tools/testing/ktest/ktest.pl
@@ -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) {
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
  2014-03-09 14:32 ` [PATCH 2/3] ktest: Some cleanup for improving readability Satoru Takeuchi
@ 2014-03-09 14:36   ` Satoru Takeuchi
  2014-03-10 16:13     ` Steven Rostedt
  0 siblings, 1 reply; 7+ messages in thread
From: Satoru Takeuchi @ 2014-03-09 14:36 UTC (permalink / raw)
  To: LKML; +Cc: Steven Rostedt

From: Satoru Takeuchi <satoru.takeuchi@gmail.com>

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

Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 tools/testing/ktest/ktest.pl | 70 +++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/tools/testing/ktest/ktest.pl b/tools/testing/ktest/ktest.pl
index c34f0de..ec82487 100755
--- a/tools/testing/ktest/ktest.pl
+++ b/tools/testing/ktest/ktest.pl
@@ -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));
-- 
1.9.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
  2014-03-09 14:36   ` [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep Satoru Takeuchi
@ 2014-03-10 16:13     ` Steven Rostedt
  2014-03-11 11:43       ` Satoru Takeuchi
  0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2014-03-10 16:13 UTC (permalink / raw)
  To: Satoru Takeuchi; +Cc: LKML

On Sun, 09 Mar 2014 23:36:49 +0900
Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:

> From: Satoru Takeuchi <satoru.takeuchi@gmail.com>
> 
> 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

Nice clean up. But there's some bugs in this patch.

 
> @@ -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.

> +    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.

>  	    }
> -	} 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;

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

>  	}
>      }
>  
> @@ -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));


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
  2014-03-10 16:13     ` Steven Rostedt
@ 2014-03-11 11:43       ` Satoru Takeuchi
  2014-03-11 11:50         ` Satoru Takeuchi
  0 siblings, 1 reply; 7+ messages in thread
From: Satoru Takeuchi @ 2014-03-11 11:43 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Satoru Takeuchi, LKML

At Mon, 10 Mar 2014 12:13:00 -0400,
Steven Rostedt wrote:
> 
> On Sun, 09 Mar 2014 23:36:49 +0900
> Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:
> 
> > From: Satoru Takeuchi <satoru.takeuchi@gmail.com>
> > 
> > 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
> 
> Nice clean up. But there's some bugs in this patch.
> 
>  
> > @@ -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.
> 
> > +    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.
> 
> >  	    }
> > -	} 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;
> 
> Here too.
> 
> In fact, with this patch, we never set to boot or more.

Sorry, my code review and test are insufficient.

> 
> 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.

> 
> Just send a fix of this patch, I have already pulled in the other two.

Thanks, I'll do.

> 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

> 
> Thanks,
> 
> -- Steve
> 
> >  	}
> >      }
> >  
> > @@ -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));
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
  2014-03-11 11:43       ` Satoru Takeuchi
@ 2014-03-11 11:50         ` Satoru Takeuchi
  2014-03-11 19:56           ` Steven Rostedt
  0 siblings, 1 reply; 7+ messages in thread
From: Satoru Takeuchi @ 2014-03-11 11:50 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Satoru Takeuchi, LKML

At Tue, 11 Mar 2014 20:43:40 +0900,
Satoru Takeuchi wrote:
> 
> At Mon, 10 Mar 2014 12:13:00 -0400,
> Steven Rostedt wrote:
> > 
> > On Sun, 09 Mar 2014 23:36:49 +0900
> > Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:
> > 
> > > From: Satoru Takeuchi <satoru.takeuchi@gmail.com>
> > > 
> > > 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
> > 
> > Nice clean up. But there's some bugs in this patch.
> > 
> >  
> > > @@ -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.
> > 
> > > +    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.
> > 
> > >  	    }
> > > -	} 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;
> > 
> > Here too.
> > 
> > In fact, with this patch, we never set to boot or more.
> 
> Sorry, my code review and test are insufficient.
> 
> > 
> > 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.
> 
> > 
> > Just send a fix of this patch, I have already pulled in the other two.
> 
> Thanks, I'll do.
> 
> > 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.

Sorry, actually tools/testing/ktest/tests.
.../examples/ is for examples for configs file as written in examples/README.

Thanks,
Satoru

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
  2014-03-11 11:50         ` Satoru Takeuchi
@ 2014-03-11 19:56           ` Steven Rostedt
  0 siblings, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2014-03-11 19:56 UTC (permalink / raw)
  To: Satoru Takeuchi; +Cc: LKML

On Tue, 11 Mar 2014 20:50:53 +0900
Satoru Takeuchi <satoru.takeuchi@gmail.com> wrote:
 
> > > 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.
> 
> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-03-11 19:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-09 14:29 [PATCH 1/3] ktest: add 2nd parameter of run_command() to set the redirect target file Satoru Takeuchi
2014-03-09 14:32 ` [PATCH 2/3] ktest: Some cleanup for improving readability Satoru Takeuchi
2014-03-09 14:36   ` [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep Satoru Takeuchi
2014-03-10 16:13     ` Steven Rostedt
2014-03-11 11:43       ` Satoru Takeuchi
2014-03-11 11:50         ` Satoru Takeuchi
2014-03-11 19:56           ` Steven Rostedt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.