Re: [PATCH 2/2][RFC]Ktest: Use dodie for critial failures
From: Steven Rostedt
Date: Thu Dec 14 2017 - 19:03:30 EST
On Tue, 21 Nov 2017 10:53:35 -0800
Tim Tianyang Chen <tianyang.chen@xxxxxxxxxx> wrote:
> Users should get emails when the script dies because of a critical failure. Critical
> failures are defined as any errors that could terminate the script, via die().
>
> In order to add email support, this patch converts all die() to dodie() except:
> * when '-v' is used as an option to get the version of the script.
> * in Sig-Int handeler because it's not a fatal error to cancel the script.
> * the die() in dodie() itself actually kills the script.
You basically converted all the "die" in the reading of the config file
into "dodie" so that you could email. But since the reading of the
config failed, there's no way to do the email, because that gets set by
the config.
Perhaps some can be converted, but most of these I would say, no.
>
> Suggested-by: Dhaval Giani <dhaval.giani@xxxxxxxxxx>
> Signed-off-by: Tim Tianyang Chen <tianyang.chen@xxxxxxxxxx>
> ---
> ktest.pl | 88 +++++++++++++++++++++++++++++++++-------------------------------
> 1 file changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/ktest.pl b/ktest.pl
> index 2e38647..763a83e 100755
> --- a/ktest.pl
> +++ b/ktest.pl
> @@ -464,9 +464,11 @@ $config_help{"REBOOT_SCRIPT"} = << "EOF"
> EOF
> ;
>
> +sub dodie;
> +
> sub _logit {
> if (defined($opt{"LOG_FILE"})) {
> - open(OUT, ">> $opt{LOG_FILE}") or die "Can't write to $opt{LOG_FILE}";
> + open(OUT, ">> $opt{LOG_FILE}") or dodie "Can't write to $opt{LOG_FILE}";
This happens at startup, and really shouldn't cause an email.
If you are worried about automation, then really whatever started ktest
should check the return result and email if it failed.
> print OUT @_;
> close(OUT);
> }
> @@ -751,7 +753,7 @@ sub set_value {
> if ($override) {
> $extra = "In the same override section!\n";
> }
> - die "$name: $.: Option $lvalue defined more than once!\n$extra";
> + dodie "$name: $.: Option $lvalue defined more than once!\n$extra";
Again, this is a failure of the parsing of the config.
> }
> ${$overrides}{$lvalue} = $prvalue;
> }
> @@ -863,7 +865,7 @@ sub process_expression {
> if ($val =~ /(.*)(==|\!=|>=|<=|>|<|=~|\!~)(.*)/) {
> my $ret = process_compare($1, $2, $3);
> if ($ret < 0) {
> - die "$name: $.: Unable to process comparison\n";
> + dodie "$name: $.: Unable to process comparison\n";
this too.
> }
> return $ret;
> }
> @@ -882,7 +884,7 @@ sub process_expression {
> return 1;
> }
>
> - die ("$name: $.: Undefined content $val in if statement\n");
> + dodie ("$name: $.: Undefined content $val in if statement\n");
ditto.
> }
>
> sub process_if {
> @@ -899,7 +901,7 @@ sub __read_config {
> my ($config, $current_test_num) = @_;
>
> my $in;
> - open($in, $config) || die "can't read file $config";
> + open($in, $config) || dodie "can't read file $config";
heh, here it failed because it couldn't even read the config file.
Hence, there's no way it knows to email anyone.
>
> my $name = $config;
> $name =~ s,.*/(.*),$1,;
> @@ -936,7 +938,7 @@ sub __read_config {
> if ($type eq "TEST_START") {
>
> if ($num_tests_set) {
> - die "$name: $.: Can not specify both NUM_TESTS and TEST_START\n";
> + dodie "$name: $.: Can not specify both NUM_TESTS and TEST_START\n";
this is a config read failure.
> }
>
> $old_test_num = $test_num;
> @@ -959,7 +961,7 @@ sub __read_config {
>
> if ($rest =~ s/\sELSE\b//) {
> if (!$if) {
> - die "$name: $.: ELSE found with out matching IF section\n$_";
> + dodie "$name: $.: ELSE found with out matching IF section\n$_";
this too.
> }
> $if = 0;
>
> @@ -997,7 +999,7 @@ sub __read_config {
> }
>
> if (!$skip && $rest !~ /^\s*$/) {
> - die "$name: $.: Gargbage found after $type\n$_";
> + dodie "$name: $.: Gargbage found after $type\n$_";
ditto.
> }
>
> if ($skip && $type eq "TEST_START") {
> @@ -1007,7 +1009,7 @@ sub __read_config {
>
> } elsif (/^\s*ELSE\b(.*)$/) {
> if (!$if) {
> - die "$name: $.: ELSE found with out matching IF section\n$_";
> + dodie "$name: $.: ELSE found with out matching IF section\n$_";
ditto.
> }
> $rest = $1;
> if ($if_set) {
> @@ -1030,7 +1032,7 @@ sub __read_config {
> }
>
> if ($rest !~ /^\s*$/) {
> - die "$name: $.: Gargbage found after DEFAULTS\n$_";
> + dodie "$name: $.: Gargbage found after DEFAULTS\n$_";
ditto.
> }
>
> } elsif (/^\s*INCLUDE\s+(\S+)/) {
> @@ -1038,7 +1040,7 @@ sub __read_config {
> next if ($skip);
>
> if (!$default) {
> - die "$name: $.: INCLUDE can only be done in default sections\n$_";
> + dodie "$name: $.: INCLUDE can only be done in default sections\n$_";
ditto.
> }
>
> my $file = process_variables($1);
> @@ -1053,7 +1055,7 @@ sub __read_config {
> }
>
> if ( ! -r $file ) {
> - die "$name: $.: Can't read file $file\n$_";
> + dodie "$name: $.: Can't read file $file\n$_";
ditto.
> }
>
> if (__read_config($file, \$test_num)) {
> @@ -1085,15 +1087,15 @@ sub __read_config {
> ($lvalue eq "NUM_TESTS" ||
> $lvalue eq "LOG_FILE" ||
> $lvalue eq "CLEAR_LOG")) {
> - die "$name: $.: $lvalue must be set in DEFAULTS section\n";
> + dodie "$name: $.: $lvalue must be set in DEFAULTS section\n";
ditto.
> }
>
> if ($lvalue eq "NUM_TESTS") {
> if ($test_num) {
> - die "$name: $.: Can not specify both NUM_TESTS and TEST_START\n";
> + dodie "$name: $.: Can not specify both NUM_TESTS and TEST_START\n";
> }
> if (!$default) {
> - die "$name: $.: NUM_TESTS must be set in default section\n";
> + dodie "$name: $.: NUM_TESTS must be set in default section\n";
ditto ditto.
> }
> $num_tests_set = 1;
> }
> @@ -1125,7 +1127,7 @@ sub __read_config {
> set_variable($lvalue, $rvalue);
>
> } else {
> - die "$name: $.: Garbage found in config\n$_";
> + dodie "$name: $.: Garbage found in config\n$_";
ditto.
> }
> }
>
> @@ -1311,7 +1313,7 @@ sub eval_option {
> # Check for recursive evaluations.
> # 100 deep should be more than enough.
> if ($r++ > 100) {
> - die "Over 100 evaluations accurred with $option\n" .
> + dodie "Over 100 evaluations accurred with $option\n" .
> "Check for recursive variables\n";
ditto.
> }
> $prev = $option;
> @@ -1442,7 +1444,7 @@ sub dodie {
> run_command $post_test;
> }
>
> - die @_, "\n";
> + die "Aborted.\n";
Total nack here. You just lost the die message. Not to mention, this
change is not related to the patch.
> }
>
> sub create_pty {
> @@ -1484,7 +1486,7 @@ sub exec_console {
> close($pts);
>
> exec $console or
> - die "Can't open console $console";
> + dodie "Can't open console $console";
OK, I'll let this change.
> }
>
> sub open_console {
> @@ -1632,7 +1634,7 @@ sub save_logs {
>
> if (!-d $dir) {
> mkpath($dir) or
> - die "can't create $dir";
> + dodie "can't create $dir";
Sure, this too.
> }
>
> my %files = (
> @@ -1645,7 +1647,7 @@ sub save_logs {
> while (my ($name, $source) = each(%files)) {
> if (-f "$source") {
> cp "$source", "$dir/$name" or
> - die "failed to copy $source";
> + dodie "failed to copy $source";
ditto (on the ok)
> }
> }
>
> @@ -1819,7 +1821,7 @@ sub get_grub2_index {
> $ssh_grub =~ s,\$SSH_COMMAND,cat $grub_file,g;
>
> open(IN, "$ssh_grub |")
> - or die "unable to get $grub_file";
> + or dodie "unable to get $grub_file";
This too.
>
> my $found = 0;
>
> @@ -1834,7 +1836,7 @@ sub get_grub2_index {
> }
> close(IN);
>
> - die "Could not find '$grub_menu' in $grub_file on $machine"
> + dodie "Could not find '$grub_menu' in $grub_file on $machine"
OK.
> if (!$found);
> doprint "$grub_number\n";
> $last_grub_menu = $grub_menu;
> @@ -1862,7 +1864,7 @@ sub get_grub_index {
> $ssh_grub =~ s,\$SSH_COMMAND,cat /boot/grub/menu.lst,g;
>
> open(IN, "$ssh_grub |")
> - or die "unable to get menu.lst";
> + or dodie "unable to get menu.lst";
OK.
>
> my $found = 0;
>
> @@ -1877,7 +1879,7 @@ sub get_grub_index {
> }
> close(IN);
>
> - die "Could not find '$grub_menu' in /boot/grub/menu on $machine"
> + dodie "Could not find '$grub_menu' in /boot/grub/menu on $machine"
OK.
> if (!$found);
> doprint "$grub_number\n";
> $last_grub_menu = $grub_menu;
> @@ -1990,7 +1992,7 @@ sub monitor {
> my $full_line = "";
>
> open(DMESG, "> $dmesg") or
> - die "unable to write to $dmesg";
> + dodie "unable to write to $dmesg";
OK.
>
> reboot_to;
>
> @@ -2878,9 +2880,9 @@ sub bisect {
>
> my $result;
>
> - die "BISECT_GOOD[$i] not defined\n" if (!defined($bisect_good));
> - die "BISECT_BAD[$i] not defined\n" if (!defined($bisect_bad));
> - die "BISECT_TYPE[$i] not defined\n" if (!defined($bisect_type));
> + dodie "BISECT_GOOD[$i] not defined\n" if (!defined($bisect_good));
> + dodie "BISECT_BAD[$i] not defined\n" if (!defined($bisect_bad));
> + dodie "BISECT_TYPE[$i] not defined\n" if (!defined($bisect_type));
OK.
>
> my $good = $bisect_good;
> my $bad = $bisect_bad;
> @@ -2966,7 +2968,7 @@ sub bisect {
>
> # checkout where we started
> run_command "git checkout $head" or
> - die "Failed to checkout $head";
> + dodie "Failed to checkout $head";
OK.
> }
>
> run_command "git bisect start$start_files" or
> @@ -3423,9 +3425,9 @@ sub patchcheck_reboot {
> sub patchcheck {
> my ($i) = @_;
>
> - die "PATCHCHECK_START[$i] not defined\n"
> + dodie "PATCHCHECK_START[$i] not defined\n"
> if (!defined($patchcheck_start));
> - die "PATCHCHECK_TYPE[$i] not defined\n"
> + dodie "PATCHCHECK_TYPE[$i] not defined\n"
> if (!defined($patchcheck_type));
OK.
>
> my $start = $patchcheck_start;
> @@ -3503,7 +3505,7 @@ sub patchcheck {
> doprint "\nProcessing commit \"$item\"\n\n";
>
> run_command "git checkout $sha1" or
> - die "Failed to checkout $sha1";
> + dodie "Failed to checkout $sha1";
OK.
>
> # only clean on the first and last patch
> if ($item eq $list[0] ||
> @@ -3594,7 +3596,7 @@ sub read_kconfig {
> }
>
> open(KIN, "$kconfig")
> - or die "Can't open $kconfig";
> + or dodie "Can't open $kconfig";
OK.
> while (<KIN>) {
> chomp;
>
> @@ -3755,7 +3757,7 @@ sub get_depends {
>
> $dep =~ s/^[^$valid]*[$valid]+//;
> } else {
> - die "this should never happen";
> + dodie "this should never happen";
OK.
> }
> }
>
> @@ -4016,7 +4018,7 @@ sub make_min_config {
> # update new ignore configs
> if (defined($ignore_config)) {
> open (OUT, ">$temp_config")
> - or die "Can't write to $temp_config";
> + or dodie "Can't write to $temp_config";
> foreach my $config (keys %save_configs) {
> print OUT "$save_configs{$config}\n";
> }
OK.
> @@ -4044,7 +4046,7 @@ sub make_min_config {
>
> # Save off all the current mandatory configs
> open (OUT, ">$temp_config")
> - or die "Can't write to $temp_config";
> + or dodie "Can't write to $temp_config";
> foreach my $config (keys %keep_configs) {
> print OUT "$keep_configs{$config}\n";
> }
OK.
> @@ -4113,7 +4115,7 @@ if ($#ARGV == 0) {
> if (! -f $ktest_config) {
> $newconfig = 1;
> get_test_case;
> - open(OUT, ">$ktest_config") or die "Can not create $ktest_config";
> + open(OUT, ">$ktest_config") or dodie "Can not create $ktest_config";
OK.
> print OUT << "EOF"
> # Generated by ktest.pl
> #
> @@ -4148,7 +4150,7 @@ if (defined($opt{"LOG_FILE"})) {
> my @new_configs = keys %entered_configs;
> if ($#new_configs >= 0) {
> print "\nAppending entered in configs to $ktest_config\n";
> - open(OUT, ">>$ktest_config") or die "Can not append to $ktest_config";
> + open(OUT, ">>$ktest_config") or dodie "Can not append to $ktest_config";
OK.
> foreach my $config (@new_configs) {
> print OUT "$config = $entered_configs{$config}\n";
> $opt{$config} = process_variables($entered_configs{$config});
> @@ -4286,11 +4288,11 @@ for (my $i = 1; $i <= $opt{"NUM_TESTS"}; $i++) {
> $outputdir = set_test_option("OUTPUT_DIR", $i);
> $builddir = set_test_option("BUILD_DIR", $i);
>
> - chdir $builddir || die "can't change directory to $builddir";
> + chdir $builddir || dodie "can't change directory to $builddir";
OK.
>
> if (!-d $outputdir) {
> mkpath($outputdir) or
> - die "can't create $outputdir";
> + dodie "can't create $outputdir";
OK.
> }
>
> $make = "$makecmd O=$outputdir";
> @@ -4321,7 +4323,7 @@ for (my $i = 1; $i <= $opt{"NUM_TESTS"}; $i++) {
>
> if (!-d $tmpdir) {
> mkpath($tmpdir) or
> - die "can't create $tmpdir";
> + dodie "can't create $tmpdir";
OK.
> }
>
> $ENV{"SSH_USER"} = $ssh_user;
> @@ -4394,7 +4396,7 @@ for (my $i = 1; $i <= $opt{"NUM_TESTS"}; $i++) {
>
> if (defined($checkout)) {
> run_command "git checkout $checkout" or
> - die "failed to checkout $checkout";
> + dodie "failed to checkout $checkout";
OK.
There, some of the changes are fine, but the first part so not.
-- Steve
> }
>
> $no_reboot = 0;
> --