smart: Remove the ATA low-power mode detection

This never worked as intended and was generally buggy, not being able to distinguish a real error code. By making the arguments less restricted this will also allow SAT protocol to be used. This commit serves as a point of git history in case someone wants to bring this functionality back.
storaged-project · Aug 23, 2023 · 3cde901 · 3cde901
1 parent 1f67b29
commit 3cde901
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 60 deletions.
diff --git a/src/lib/plugin_apis/smart.api b/src/lib/plugin_apis/smart.api
@@ -15,14 +15,12 @@ GQuark bd_smart_error_quark (void) {
  * @BD_SMART_ERROR_TECH_UNAVAIL: SMART support not available.
  * @BD_SMART_ERROR_FAILED: General error.
  * @BD_SMART_ERROR_INVALID_ARGUMENT: Invalid argument.
- * @BD_SMART_ERROR_DRIVE_SLEEPING: Device is in a low-power mode.
  */
 /* BpG-skip-end */
 typedef enum {
     BD_SMART_ERROR_TECH_UNAVAIL,
     BD_SMART_ERROR_FAILED,
     BD_SMART_ERROR_INVALID_ARGUMENT,
-    BD_SMART_ERROR_DRIVE_SLEEPING,
 } BDSmartError;
 
 typedef enum {
@@ -565,21 +563,15 @@ typedef enum {
 /**
  * bd_smart_ata_get_info:
  * @device: device to check.
- * @nowakeup: prevent drive waking up if in a low-power mode.
  * @error: (out) (optional): place to store error (if any).
  *
  * Retrieve SMART information from the drive.
  *
- * Specify @nowakeup to prevent drive spinning up when in a low-power mode,
- * #BD_SMART_ERROR_DRIVE_SLEEPING will be returned in such case. Note that
- * smartctl may actually return this error on non-ATA devices or when
- * device identification fails.
- *
  * Returns: (transfer full): ATA SMART log or %NULL in case of an error (with @error set).
  *
  * Tech category: %BD_SMART_TECH_ATA-%BD_SMART_TECH_MODE_INFO
  */
-BDSmartATA * bd_smart_ata_get_info (const gchar *device, gboolean nowakeup, GError **error);
+BDSmartATA * bd_smart_ata_get_info (const gchar *device, GError **error);
 
 /**
  * bd_smart_scsi_get_info:

diff --git a/src/plugins/smart.c b/src/plugins/smart.c
@@ -340,10 +340,9 @@ static gchar ** parse_error_messages (JsonReader *reader) {
 }
 
 
-#define STANDBY_RET_CODE 255      /* custom return code to indicate sleeping drive */
 #define MIN_JSON_FORMAT_VER 1     /* minimal json_format_version */
 
-static gboolean parse_smartctl_error (gint wait_status, const gchar *stdout, const gchar *stderr, gboolean nowakeup, JsonParser *parser, GError **error) {
+static gboolean parse_smartctl_error (gint wait_status, const gchar *stdout, const gchar *stderr, JsonParser *parser, GError **error) {
     gint status = 0;
     gint res;
     JsonReader *reader;
@@ -364,11 +363,6 @@ static gboolean parse_smartctl_error (gint wait_status, const gchar *stdout, con
         g_clear_error (&l_error);
     }
 
-    if (nowakeup && status == STANDBY_RET_CODE) {
-        g_set_error_literal (error, BD_SMART_ERROR, BD_SMART_ERROR_DRIVE_SLEEPING,
-                             "Device is in a low-power mode");
-        return FALSE;
-    }
     if ((!stdout || strlen (stdout) == 0) &&
         (!stderr || strlen (stderr) == 0)) {
         g_set_error_literal (error, BD_SMART_ERROR, BD_SMART_ERROR_FAILED,
@@ -1080,32 +1074,23 @@ static BDSmartSCSI * parse_scsi_smart (JsonParser *parser, G_GNUC_UNUSED GError
 /**
  * bd_smart_ata_get_info:
  * @device: device to check.
- * @nowakeup: prevent drive waking up if in a low-power mode.
  * @error: (out) (optional): place to store error (if any).
  *
  * Retrieve SMART information from the drive.
  *
- * Specify @nowakeup to prevent drive spinning up when in a low-power mode,
- * #BD_SMART_ERROR_DRIVE_SLEEPING will be returned in such case. Note that
- * smartctl may actually return this error on non-ATA devices or when
- * device identification fails.
- *
  * Returns: (transfer full): ATA SMART log or %NULL in case of an error (with @error set).
  *
  * Tech category: %BD_SMART_TECH_ATA-%BD_SMART_TECH_MODE_INFO
  */
-BDSmartATA * bd_smart_ata_get_info (const gchar *device, gboolean nowakeup, GError **error) {
-    const gchar *args[11] = { "smartctl", "--info", "--health", "--capabilities", "--attributes", "--json", "--nocheck=never", "--device=ata" /* TODO */, "--badsum=ignore", device, NULL };
+BDSmartATA * bd_smart_ata_get_info (const gchar *device, GError **error) {
+    const gchar *args[8] = { "smartctl", "--info", "--health", "--capabilities", "--attributes", "--json", device, NULL };
     gint wait_status = 0;
     gchar *stdout = NULL;
     gchar *stderr = NULL;
     JsonParser *parser;
     BDSmartATA *data = NULL;
     gboolean ret;
 
-    if (nowakeup)
-        args[6] = "--nocheck=standby," G_STRINGIFY (STANDBY_RET_CODE);
-
     /* TODO: set UTF-8 locale for JSON? */
     if (!g_spawn_sync (NULL /* working_directory */,
                        (gchar **) args,
@@ -1127,7 +1112,7 @@ BDSmartATA * bd_smart_ata_get_info (const gchar *device, gboolean nowakeup, GErr
         g_strstrip (stderr);
 
     parser = json_parser_new ();
-    ret = parse_smartctl_error (wait_status, stdout, stderr, nowakeup, parser, error);
+    ret = parse_smartctl_error (wait_status, stdout, stderr, parser, error);
     g_free (stdout);
     g_free (stderr);
     if (! ret) {
@@ -1184,7 +1169,7 @@ BDSmartSCSI * bd_smart_scsi_get_info (const gchar *device, GError **error) {
         g_strstrip (stderr);
 
     parser = json_parser_new ();
-    ret = parse_smartctl_error (wait_status, stdout, stderr, FALSE, parser, error);
+    ret = parse_smartctl_error (wait_status, stdout, stderr, parser, error);
     g_free (stdout);
     g_free (stderr);
     if (! ret) {
@@ -1244,7 +1229,7 @@ gboolean bd_smart_set_enabled (const gchar *device, gboolean enabled, GError **e
         g_strstrip (stderr);
 
     parser = json_parser_new ();
-    ret = parse_smartctl_error (wait_status, stdout, stderr, FALSE, parser, error);
+    ret = parse_smartctl_error (wait_status, stdout, stderr, parser, error);
     g_free (stdout);
     g_free (stderr);
     g_object_unref (parser);
@@ -1319,7 +1304,7 @@ gboolean bd_smart_device_self_test (const gchar *device, BDSmartSelfTestOp opera
         g_strstrip (stderr);
 
     parser = json_parser_new ();
-    ret = parse_smartctl_error (wait_status, stdout, stderr, FALSE, parser, error);
+    ret = parse_smartctl_error (wait_status, stdout, stderr, parser, error);
     g_free (stdout);
     g_free (stderr);
     g_object_unref (parser);

diff --git a/src/plugins/smart.h b/src/plugins/smart.h
@@ -12,13 +12,11 @@ GQuark bd_smart_error_quark (void);
  * @BD_SMART_ERROR_TECH_UNAVAIL: SMART support not available.
  * @BD_SMART_ERROR_FAILED: General error.
  * @BD_SMART_ERROR_INVALID_ARGUMENT: Invalid argument.
- * @BD_SMART_ERROR_DRIVE_SLEEPING: Device is in a low-power mode.
  */
 typedef enum {
     BD_SMART_ERROR_TECH_UNAVAIL,
     BD_SMART_ERROR_FAILED,
     BD_SMART_ERROR_INVALID_ARGUMENT,
-    BD_SMART_ERROR_DRIVE_SLEEPING,
 } BDSmartError;
 
 typedef enum {
@@ -413,7 +411,6 @@ gboolean bd_smart_is_tech_avail (BDSmartTech tech, guint64 mode, GError **error)
 
 
 BDSmartATA *   bd_smart_ata_get_info     (const gchar        *device,
-                                          gboolean            nowakeup,
                                           GError            **error);
 BDSmartSCSI *  bd_smart_scsi_get_info    (const gchar        *device,
                                           GError            **error);

diff --git a/tests/smart_test.py b/tests/smart_test.py
@@ -87,42 +87,30 @@ def test_ata_info(self):
             raise unittest.SkipTest("smartctl executable not found in $PATH, skipping.")
 
         # non-existing device
-        msg = r".*Error getting ATA SMART info: Smartctl open device: /dev/.* failed: No such device"
+        msg = r".*Error getting ATA SMART info: /dev/.*: Unable to detect device type"
         with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info("/dev/nonexistent", False)
-        with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info("/dev/nonexistent", True)
+            BlockDev.smart_ata_get_info("/dev/nonexistent")
 
         # LIO device (SCSI)
         self._setup_lio()
         self.addCleanup(self._clean_lio)
-        msg = r"Error getting ATA SMART info: Device open failed or device did not return an IDENTIFY DEVICE structure."
-        with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info(self.lio_dev, False)
-        msg = r"Device is in a low-power mode"     # FIXME
+        msg = r"Missing 'ata_smart_data' section: The member .ata_smart_data. is not defined in the object at the current position."
         with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info(self.lio_dev, True)
+            BlockDev.smart_ata_get_info(self.lio_dev)
 
         # loop device
         self._setup_loop()
         self.addCleanup(self._clean_loop)
-        # Sadly there's no specific message reported for loop devices (not being an ATA device)
-        msg = r"Error getting ATA SMART info: Device open failed or device did not return an IDENTIFY DEVICE structure."
+        msg = r".*Error getting ATA SMART info: /dev/.*: Unable to detect device type"
         with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info(self.loop_dev, False)
-        msg = r"Device is in a low-power mode"     # FIXME
-        with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info(self.loop_dev, True)
+            BlockDev.smart_ata_get_info(self.loop_dev)
 
         # scsi_debug
         self._setup_scsi_debug()
         self.addCleanup(self._clean_scsi_debug)
-        msg = r"Error getting ATA SMART info: Device open failed or device did not return an IDENTIFY DEVICE structure."
-        with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info(self.scsi_debug_dev, False)
-        msg = r"Device is in a low-power mode"     # FIXME
+        msg = r"Missing 'ata_smart_data' section: The member .ata_smart_data. is not defined in the object at the current position."
         with self.assertRaisesRegexp(GLib.GError, msg):
-            BlockDev.smart_ata_get_info(self.loop_dev, True)
+            BlockDev.smart_ata_get_info(self.scsi_debug_dev)
 
 
     @tag_test(TestTags.CORE)
@@ -132,7 +120,7 @@ def test_ata_real_dumps(self):
         with fake_utils("tests/fake_utils/smartctl"):
             for d in ["TOSHIBA_THNSNH128GBST", "Hitachi_HDS5C3020ALA632", "HGST_HMS5C4040BLE640",
                       "HGST_HUS726060ALA640", "INTEL_SSDSC2BB120G4L", "WDC_WD10EFRX-68PJCN0"]:
-                data = BlockDev.smart_ata_get_info(d, False)
+                data = BlockDev.smart_ata_get_info(d)
                 self.assertIsNotNone(data)
                 self.assertTrue(data.overall_status_passed)
                 self.assertGreater(data.power_cycle_count, 0)
@@ -154,22 +142,22 @@ def test_ata_error_dumps(self):
         with fake_utils("tests/fake_utils/smartctl"):
             msg = r"Reported smartctl JSON format version too low: 0"
             with self.assertRaisesRegexp(GLib.GError, msg):
-                BlockDev.smart_ata_get_info("01_old_ver", False)
+                BlockDev.smart_ata_get_info("01_old_ver")
 
             msg = r"Error getting ATA SMART info: Command line did not parse."
             with self.assertRaisesRegexp(GLib.GError, msg):
-                BlockDev.smart_ata_get_info("02_exit_err", False)
+                BlockDev.smart_ata_get_info("02_exit_err")
 
-            data = BlockDev.smart_ata_get_info("03_exit_err_32", False)
+            data = BlockDev.smart_ata_get_info("03_exit_err_32")
             self.assertIsNotNone(data)
 
             msg = r"Error getting ATA SMART info: .* Parse error: unexpected character"
             with self.assertRaisesRegexp(GLib.GError, msg):
-                BlockDev.smart_ata_get_info("04_malformed", False)
+                BlockDev.smart_ata_get_info("04_malformed")
 
             msg = r"Empty response"
             with self.assertRaisesRegexp(GLib.GError, msg):
-                BlockDev.smart_ata_get_info("05_empty", False)
+                BlockDev.smart_ata_get_info("05_empty")
 
 
     @tag_test(TestTags.CORE)