[pve-devel] [PATCH storage 1/2] merge get_smart_data/health

[Dominik Csapak]

instead of parsing the output of smart in two places,
give get_smart_data a flag if we only want health

this fixes a bug (not on the bugtracker), where
an ssd with disabled smart had an empty string as health
in the gui

Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
---
 PVE/API2/Disks.pm |  7 ++-----
 PVE/Diskmanage.pm | 42 +++++++++++-------------------------------
 2 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
index 208485f..366cd62 100644
--- a/PVE/API2/Disks.pm
+++ b/PVE/API2/Disks.pm
@@ -140,13 +140,10 @@ __PACKAGE__->register_method ({
 
 	my $result = {};
 
-	if ($param->{healthonly}) {
-	    $result = { health => PVE::Diskmanage::get_smart_health($disk) };
-	} else {
-	    $result = PVE::Diskmanage::get_smart_data($disk);
-	}
+	my $result = PVE::Diskmanage::get_smart_data($disk, $param->{healthonly});
 
 	$result->{health} = 'UNKNOWN' if !defined $result->{health};
+	$result = { health => $result->{health} } if $param->{healthonly};
 
 	return $result;
     }});
diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 36a5b5b..4ee89ae 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -70,7 +70,7 @@ sub disk_is_used {
 }
 
 sub get_smart_data {
-    my ($disk) = @_;
+    my ($disk, $healthonly) = @_;
 
     assert_blockdev($disk);
     my $smartdata = {};
@@ -81,8 +81,12 @@ sub get_smart_data {
     $disk =~ s/n\d+$//
         if $disk =~ m!^/dev/nvme\d+n\d+$!;
 
+    my $cmd = [$SMARTCTL, '-H'];
+    push @$cmd, '-A', '-f', 'brief' if !$healthonly;
+    push @$cmd, $disk;
+
     eval {
-	$returncode = run_command([$SMARTCTL, '-H', '-A', '-f', 'brief', $disk], noerr => 1, outfunc => sub{
+	$returncode = run_command($cmd, noerr => 1, outfunc => sub{
 	    my ($line) = @_;
 
 # ATA SMART attributes, e.g.:
@@ -115,6 +119,8 @@ sub get_smart_data {
 	    } elsif (defined($type) && $type eq 'text') {
 		$smartdata->{text} = '' if !defined $smartdata->{text};
 		$smartdata->{text} .= "$line\n";
+	    } elsif ($line =~ m/SMART Disabled/) {
+		$smartdata->{health} = "SMART Disabled";
 	    }
 	});
     };
@@ -132,30 +138,6 @@ sub get_smart_data {
     return $smartdata;
 }
 
-sub get_smart_health {
-    my ($disk) = @_;
-
-    return "NOT A DEVICE" if !assert_blockdev($disk, 1);
-
-    my $message;
-    $disk =~ s/n\d+$//
-        if $disk =~ m!^/dev/nvme\d+n\d+$!;
-
-    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";
-	}
-    });
-
-    return $message;
-}
-
 sub get_zfs_devices {
     my $list = {};
 
@@ -388,11 +370,12 @@ sub get_disks {
 
 	if (!$nosmart) {
 	    eval {
+		my $smartdata = get_smart_data($devpath, ($type ne 'ssd'));
+		$health = $smartdata->{health} if $smartdata->{health};
+
 		if ($type eq 'ssd') {
 		    # 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
@@ -403,9 +386,6 @@ sub get_disks {
 			# prefer the 233 value
 			last if ($attr->{id} == 233);
 		    }
-		} else {
-		    # else we just get the health
-		    $health = get_smart_health($devpath);
 		}
 	    };
 	}
-- 
2.1.4