[pve-devel] [PATCH v2 storage] fix #1123: move SMART error handling into get_disks

[Fabian Grünbichler]

because we never ever want to die there because of a single
disk, but the nodes/xyz/disks/smart API path is allowed to
fail if a disk device is unsupported by smartctl or
something else goes wrong.
---
Changes to v1:

handle fatal errors in get_disks instead of in get_smart_*

 PVE/API2/Disks.pm |  2 ++
 PVE/Diskmanage.pm | 69 ++++++++++++++++++++++++++-----------------------------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
index 279d351..d7030f8 100644
--- a/PVE/API2/Disks.pm
+++ b/PVE/API2/Disks.pm
@@ -138,6 +138,8 @@ __PACKAGE__->register_method ({
 	    $result = PVE::Diskmanage::get_smart_data($disk);
 	}
 
+	$result->{health} = 'UNKOWN' if !defined $result->{health};
+
 	return $result;
     }});
 
diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index bee5d99..5909d73 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -110,7 +110,6 @@ sub get_smart_data {
     if ((defined($returncode) && ($returncode & 0b00000011)) || $err) {
 	die "Error getting S.M.A.R.T. data: Exit code: $returncode\n";
     }
-    $smartdata->{health} = 'UNKOWN' if !defined $smartdata->{health};
     return $smartdata;
 }
 
@@ -119,22 +118,19 @@ sub get_smart_health {
 
     return "NOT A DEVICE" if !assert_blockdev($disk, 1);
 
-    my $message = "UNKOWN";
+    my $message;
 
-    eval {
-	run_command([$SMARTCTL, '-H', $disk], noerr => 1, outfunc => sub {
-	    my ($line) = @_;
+    run_command([$SMARTCTL, '-H', $disk], noerr => 1, outfunc => sub {
+	my ($line) = @_;
 
-	    if ($line =~ m/test result: (.*)$/) {
-		$message = $1;
-	    } elsif ($line =~ m/open device: (.*) failed: (.*)$/) {
-		$message = "FAILED TO OPEN";
-	    } elsif ($line =~ m/^SMART Disabled/) {
-		$message = "SMART DISABLED";
-	    }
-	});
-    };
-    # we ignore errors here because by default we want to return UNKNOWN
+	if ($line =~ m/test result: (.*)$/) {
+	    $message = $1;
+	} elsif ($line =~ m/open device: (.*) failed: (.*)$/) {
+	    $message = "FAILED TO OPEN";
+	} elsif ($line =~ m/^SMART Disabled/) {
+	    $message = "SMART DISABLED";
+	}
+    });
 
     return $message;
 }
@@ -366,29 +362,30 @@ sub get_disks {
 	    }
 	}
 
-	my $health;
+	my $health = 'UNKNOWN';
 	my $wearout;
-	if ($type eq 'ssd' && !defined($disk)) {
-	    # if we have an ssd we try to get the wearout indicator
-	    my $smartdata = get_smart_data($devpath);
-	    $health = $smartdata->{health};
-	    foreach my $attr (@{$smartdata->{attributes}}) {
-		# ID 233 is media wearout indicator on intel and sandisk
-		# ID 177 is media wearout indicator on samsung
-		next if ($attr->{id} != 233 && $attr->{id} != 177);
-		next if ($attr->{name} !~ m/wear/i);
-		$wearout = $attr->{value};
-
-		# prefer the 233 value
-		last if ($attr->{id} == 233);
+	eval {
+	    if ($type eq 'ssd' && !defined($disk)) {
+		# if we have an ssd we try to get the wearout indicator
+		$wearout = 'N/A';
+		my $smartdata = get_smart_data($devpath);
+		$health = $smartdata->{health};
+		foreach my $attr (@{$smartdata->{attributes}}) {
+		    # ID 233 is media wearout indicator on intel and sandisk
+		    # ID 177 is media wearout indicator on samsung
+		    next if ($attr->{id} != 233 && $attr->{id} != 177);
+		    next if ($attr->{name} !~ m/wear/i);
+		    $wearout = $attr->{value};
+
+		    # prefer the 233 value
+		    last if ($attr->{id} == 233);
+		}
+	    } elsif (!defined($disk)) {
+		# we do not need smart data if we check a single disk
+		# because this functionality is only for disk_is_used
+		$health = get_smart_health($devpath) if !defined($disk);
 	    }
-
-	    $wearout = 'N/A' if !defined($wearout);
-	} elsif (!defined($disk)) {
-	    # we do not need smart data if we check a single disk
-	    # because this functionality is only for disk_is_used
-	    $health = get_smart_health($devpath) if !defined($disk);
-	}
+	};
 
 	my $used;
 
-- 
2.1.4