Refactor lsblk, resolves #2907

scan_disks() raised ValueError when lsblk values had leading spaces refactored to strip leading and trailing spaces - Adding .python-version to .gitignore to support local pyenv - Add new line to test_scan_disks_lsblk_parse_fail with leading space - Remove old comment about issue with leading space
rockstor · Sep 30, 2024 · 366a026 · 366a026
1 parent 1ddcf4b
commit 366a026
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 18 deletions.
diff --git a/.gitignore b/.gitignore
@@ -61,3 +61,6 @@ rockstor-tasks-huey.db*
 # Poetry error logs
 poetry-installer-error*
 /build-output.txt
+
+# Pyenv version
+.python-version
diff --git a/src/rockstor/system/osi.py b/src/rockstor/system/osi.py
@@ -326,19 +326,20 @@ def scan_disks(min_size: int, test_mode: bool = False) -> list[Disk]:
         if line == "" or re.match("NAME", line) is None:
             continue
         # lsblk example line (incomplete): 'NAME="/dev/sdb" MODEL="TOSHIBA MK1652GS" VENDOR="ATA     " LABEL="" UUID=""'
-        # line.strip().split('" ') = ['NAME="/dev/sdb', 'MODEL="TOSHIBA MK1652GS', 'VENDOR="ATA     ', 'LABEL="', 'UUID=""']   # noqa E501
-        # Device information built from each lsblk line in turn.
-        clean_line = re.sub('"\s+"', '""', line).strip()
-        # logger.debug(f"Scan_disks() using lsblk clean_line={clean_line}")
+        # Device information built from each lsblk line in turn
+        # 'KEY_1="VALUE_1" KEY_2="VALUE_2" ... KEY_N="VALUE_N"' will become
+        # ['KEY_1', '', 'VALUE_1', 'KEY_2', '', 'VALUE_2', ... 'KEY_N', '', 'VALUE_N', '']
+        line = [item.strip() for item in re.split(r'["=]', line)]
+        # Remove every third value starting from index 2 to turn the list into
+        # ['KEY_1', 'VALUE_1', 'KEY_2', 'VALUE_2', 'KEY_N', 'VALUE_N', '']
+        # Trailing item is ok due to how zip is done
+        line = [item for i, item in enumerate(line, 2) if i % 3 != 0]
+        # logger.debug(f"Scan_disks() using lsblk line={line}")
         blk_dev_properties: dict = {
-            key.lower()
-            if key != "TRAN"
-            else "transport": value.replace('"', "").strip()
-            if value.replace('"', "").strip() != ""
-            else None
-            for key, value in (
-                key_value.split("=") for key_value in clean_line.split('" ')
+            key.lower() if key != "TRAN" else "transport": (
+                value if value != "" else None
             )
+            for key, value in zip(line[::2], line[1::2])
         }
         logger.debug(f"Scan_disks() using: blk_dev_properties={blk_dev_properties}")
         # Disk namedtuple from lsblk line dictionary.
@@ -1995,8 +1996,8 @@ def get_devname(device_name, addPath=False):
 
 def get_libs(program_path: str) -> list[str]:
     """
-    Wrapper around `ldd program_path` 
-    @param program_path: Binary to query ldd about 
+    Wrapper around `ldd program_path`
+    @param program_path: Binary to query ldd about
     @return: list of OS paths or empty list if error or non found.
     """
     libs: list[str] = []

diff --git a/src/rockstor/system/tests/test_osi.py b/src/rockstor/system/tests/test_osi.py
@@ -1650,11 +1650,6 @@ def test_scan_disks_lsblk_parse_fail(self):
         cd /opt/rockstor/src/rockstor/system/tests
         /opt/rockstor/.venv/bin/python -m unittest test_osi.OSITests.test_scan_disks_lsblk_parse_fail
         """
-        # E.g 'VENDOR="        "' failed with: "ValueError: not enough values to unpack (expected 2, got 1)"
-        # We use dictionary comprehension to parse lsblk output and rely on: 'line.strip().split('" ')'
-        # this breaks when a disk property begins with a space.
-        # See: https://github.com/rockstor/rockstor-core/issues/2853
-        # Reported/encountered/addressed failure concerns values consisting solely of whitespace.
         out = [
             [
                 'NAME="/dev/fd0" MODEL="" SERIAL="" SIZE="4K" TRAN="" VENDOR="" HCTL="" TYPE="disk" FSTYPE="" LABEL="" UUID=""',  # noqa E501
@@ -1664,6 +1659,7 @@ def test_scan_disks_lsblk_parse_fail(self):
                 'NAME="/dev/sdb2" MODEL="" SERIAL="" SIZE="64M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="vfat" LABEL="EFI" UUID="1381-2428"',  # noqa E501
                 'NAME="/dev/sdb3" MODEL="" SERIAL="" SIZE="2G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="swap" LABEL="SWAP" UUID="ee6de8d5-eb1c-4d19-943c-0d24ecee92a7"',  # noqa E501
                 'NAME="/dev/sdb4" MODEL="" SERIAL="" SIZE="29.9G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="btrfs" LABEL="ROOT" UUID="46befa9e-b399-480d-b10d-338a4c875615"',  # noqa E501
+                'NAME="/dev/sdd" MODEL="SanDisk 3.2Gen1" SERIAL="01016c73a2fabf22051f78f30546627c9fb5e8083fbc14d477f85d6a269220dd15710000000000000000000060a25f5700804f0091558107a3ab6843" SIZE="57.3G" TRAN="usb" VENDOR=" USB    " HCTL="6:0:0:0" TYPE="disk" FSTYPE="" LABEL="" UUID=""',  # noqa E501
                 "",
             ]
         ]
@@ -1703,6 +1699,22 @@ def test_scan_disks_lsblk_parse_fail(self):
                     root=False,
                     partitions={"/dev/sdb4": "btrfs"},
                 ),
+                Disk(
+                    name="/dev/sdd",
+                    model="SanDisk 3.2Gen1",
+                    serial="01016c73a2fabf22051f78f30546627c9fb5e8083fbc14d477f85d6a269220dd15710000000000000000000060a25f5700804f0091558107a3ab6843",
+                    size=60083404,
+                    transport="usb",
+                    vendor="USB",
+                    hctl="6:0:0:0",
+                    type="disk",
+                    fstype=None,
+                    label=None,
+                    uuid=None,
+                    parted=False,
+                    root=False,
+                    partitions={}
+                )
             ]
         ]
         # As all serials are available via the lsblk we can avoid mocking