Re: [PATCH] ktest: Add workaround to avoid unexpected power cycle

[Masayoshi Mizuma]

On Wed, Apr 17, 2019 at 04:29:42PM -0400, Steven Rostedt wrote:
> On Wed, 17 Apr 2019 16:14:42 -0400
> Masayoshi Mizuma <msys.mizuma@xxxxxxxxx> wrote:
> 
> > From: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> > 
> > Unexpected power cycle occurs while the installation of the
> > kernel.
> > 
> > That is because the default reboot command, "ssh $SSH_USER@$MACHINE
> > reboot" exits as 255 even if the reboot is successfully done,
> > like as:
> > 
> >   ]# ssh root@Test reboot
> >   Connection to Test closed by remote host.
> >   ]# echo $?
> >   255
> >   ]#
> > 
> > To avoid the unexpected power cycle, the reboot is considered as
> > successfully done if the reboot is the default command and the
> > return code is 255.
> > 
> 
> Ah that explains why I've been seeing this :-)
> 
> Can we add a config modifying variable called:
> 
> REBOOT_RETURN_CODE
> 
> that is by default 255 and can be changed by the config file?
> 
> You just need to add in %default:
> 
> 	"REBOOT_RETURN_CODE"	=> 255,
> 
> my $reboot_return_code;
> 
> 
> and in %option_map:
> 
> 	"REBOOT_RETURN_CODE"	=> \$reboot_return_code,

Great idea, thanks! I'll add thease.

> 
> 
> > Signed-off-by: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> > ---
> >  tools/testing/ktest/ktest.pl | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/testing/ktest/ktest.pl b/tools/testing/ktest/ktest.pl
> > index ea07d43856b8..765c6bc83ab4 100755
> > --- a/tools/testing/ktest/ktest.pl
> > +++ b/tools/testing/ktest/ktest.pl
> > @@ -1737,6 +1737,11 @@ sub run_command {
> >      my $dord = 0;
> >      my $dostdout = 0;
> >      my $pid;
> > +    my $is_default_reboot = 0;
> > +
> > +    if ($command eq $default{REBOOT}) {
> > +	$is_default_reboot = 1;
> > +    }
> 
> Do we really need to add this variable?
> 

> >  
> >      $command =~ s/\$SSH_USER/$ssh_user/g;
> >      $command =~ s/\$MACHINE/$machine/g;

$command is modified here, so...

> > @@ -1791,6 +1796,10 @@ sub run_command {
> >      # shift 8 for real exit status
> >      $run_command_status = $? >> 8;
> >  
> > +    if ($run_command_status == 255 && $is_default_reboot) {
> 
> Instead can we have:
> 
> 	if ($command eq $default{REBOOT} &&
> 	    $run_command_status == $reboot_return_code) {
> 
> ?

How about the following?

@@ -1737,6 +1740,7 @@ sub run_command {
     my $dord = 0;
     my $dostdout = 0;
     my $pid;
+    my $command_orig = $command;

     $command =~ s/\$SSH_USER/$ssh_user/g;
     $command =~ s/\$MACHINE/$machine/g;
@@ -1791,6 +1795,11 @@ sub run_command {
     # shift 8 for real exit status
     $run_command_status = $? >> 8;

+    if ($command_orig eq $default{REBOOT} &&
+       $run_command_status == $reboot_return_code) {
+       $run_command_status = 0;
+    }
+
     close(CMD);
     close(LOG) if ($dolog);
     close(RD)  if ($dord);

Thanks!
Masa