lkml.org 
[lkml]   [2014]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] ktest: remove the misleading $buildonly and introduce $laststep.
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));
>


\
 
 \ /
  Last update: 2014-03-11 13:21    [W:0.095 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site