From 910503b641c92199086100341ee5bbc9782dd565 Mon Sep 17 00:00:00 2001 From: Roland Date: Mon, 6 Jul 2026 16:52:11 -0600 Subject: [PATCH] Reconciler: uid/gid and ntfsfix for NTFS secondary mounts. QA on 10.0.0.8 (mount-issues.md findings 2 and 3) showed the reconciler path missed both NTFS accommodations that mount_drive() and add_fstab_entry() already have: secondary NTFS drives mounted owned by root (writable only via ntfs-3g's permissive default), and a dirty NTFS volume would fail to mount at boot with no cleanup attempted. Inject uid/gid for ntfs/ntfs3 in the reconciler, and run ntfsfix -d in SubprocessMountExecutor before NTFS mounts (translating UUID=/LABEL= specs to /dev/disk/by-* paths, which ntfsfix needs). Also return an error MountResult instead of raising when a binary is missing, per the executor protocol. Co-Authored-By: Claude Fable 5 --- controller/lib/mount_executor.py | 27 ++++++++++++ controller/lib/reconciler.py | 6 +-- controller/test/test_mount_executor.py | 61 ++++++++++++++++++++++++++ controller/test/test_reconciler.py | 11 +++++ 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/controller/lib/mount_executor.py b/controller/lib/mount_executor.py index 79e83688..9cdd2dcb 100644 --- a/controller/lib/mount_executor.py +++ b/controller/lib/mount_executor.py @@ -59,6 +59,18 @@ class MountExecutor(Protocol): MOUNT_TIMEOUT_SECONDS = 30 +def _device_path(device: str) -> str: + """Translate a UUID=/LABEL= mount spec to a real device node. + + mount(8) resolves these specs itself, but tools like ntfsfix need a + path. udev maintains the by-uuid/by-label symlinks.""" + if device.startswith("UUID="): + return f"/dev/disk/by-uuid/{device[len('UUID='):]}" + if device.startswith("LABEL="): + return f"/dev/disk/by-label/{device[len('LABEL='):]}" + return device + + class SubprocessMountExecutor: """Executes mount/umount via subprocess and parses /proc/mounts.""" @@ -72,6 +84,17 @@ class SubprocessMountExecutor: fstype: str, options: str, ) -> MountResult: + # NTFS volumes are frequently left "dirty" (Windows Fast Startup, or + # an unclean unplug), which makes the driver refuse to mount them. + # ntfsfix -d clears the dirty flag and replays the journal; it is a + # no-op on clean volumes. Never fatal: the mount below reports the + # real error. Matches mount_drive() in lib/disks.py. + if fstype in ("ntfs", "ntfs3"): + fix = self._run(["ntfsfix", "-d", _device_path(device)], + label=f"ntfsfix {device}") + if not fix.ok: + logger.warning("ntfsfix failed for %s (mount will still be attempted): %s", + device, fix.error) cmd = ["mount", "-t", fstype, "-o", options, device, mount_point] return self._run(cmd, label=f"mount {device} {mount_point}") @@ -104,6 +127,10 @@ class SubprocessMountExecutor: ok=False, error=f"{label}: timed out after {self._timeout}s", ) + except OSError as e: + # e.g. FileNotFoundError for a missing binary — the executor + # protocol forbids raising for I/O failures. + return MountResult(ok=False, error=f"{label}: {e}") if result.returncode != 0: return MountResult( ok=False, diff --git a/controller/lib/reconciler.py b/controller/lib/reconciler.py index 8b218d3c..18590a6a 100644 --- a/controller/lib/reconciler.py +++ b/controller/lib/reconciler.py @@ -149,11 +149,11 @@ class Reconciler: return None def _inject_uid_gid(self, entry: FstabEntry) -> str: - """exfat/vfat have no POSIX permissions; without uid/gid the + """exfat/vfat/ntfs have no POSIX permissions; without uid/gid the mounted directory belongs to root and the wrolpi user cannot write - to it.""" + to it. Covers both NTFS drivers, matching mount_drive().""" options = entry.options or "defaults" - if entry.fstype not in ("exfat", "vfat"): + if entry.fstype not in ("exfat", "vfat", "ntfs", "ntfs3"): return options if "uid=" in options or "gid=" in options: return options diff --git a/controller/test/test_mount_executor.py b/controller/test/test_mount_executor.py index 55a0a9e5..f938d69a 100644 --- a/controller/test/test_mount_executor.py +++ b/controller/test/test_mount_executor.py @@ -124,6 +124,67 @@ class TestSubprocessMountExecutor: assert calls == [["mount", "-t", "ext4", "-o", "defaults", "UUID=1", "/media/x"]] + def test_ntfs_mount_runs_ntfsfix_first(self, monkeypatch): + """NTFS mounts run ntfsfix -d first to clear the Windows dirty flag; + UUID= specs are translated to /dev/disk/by-uuid for ntfsfix.""" + calls = [] + + def fake_run(cmd, **kwargs): + calls.append(cmd) + return _FakeCompletedProcess(returncode=0) + + monkeypatch.setattr( + "controller.lib.mount_executor.subprocess.run", fake_run, + ) + executor = SubprocessMountExecutor() + result = executor.mount("UUID=ABC123", "/media/x", "ntfs", "defaults") + assert result == MountResult(ok=True) + assert calls[0] == ["ntfsfix", "-d", "/dev/disk/by-uuid/ABC123"] + assert calls[1][0] == "mount" + + def test_ntfsfix_failure_does_not_block_mount(self, monkeypatch): + def fake_run(cmd, **kwargs): + if cmd[0] == "ntfsfix": + return _FakeCompletedProcess(returncode=1, stderr="dirty") + return _FakeCompletedProcess(returncode=0) + + monkeypatch.setattr( + "controller.lib.mount_executor.subprocess.run", fake_run, + ) + executor = SubprocessMountExecutor() + result = executor.mount("/dev/sda1", "/media/x", "ntfs3", "defaults") + assert result == MountResult(ok=True) + + def test_ext4_mount_does_not_run_ntfsfix(self, monkeypatch): + calls = [] + + def fake_run(cmd, **kwargs): + calls.append(cmd) + return _FakeCompletedProcess(returncode=0) + + monkeypatch.setattr( + "controller.lib.mount_executor.subprocess.run", fake_run, + ) + executor = SubprocessMountExecutor() + executor.mount("UUID=1", "/media/x", "ext4", "defaults") + assert len(calls) == 1 + assert calls[0][0] == "mount" + + def test_missing_binary_returns_error_instead_of_raising(self, monkeypatch): + """The executor protocol forbids raising for I/O failures; a missing + binary (FileNotFoundError) must come back as MountResult(ok=False).""" + + def fake_run(cmd, **kwargs): + raise FileNotFoundError(2, "No such file or directory", cmd[0]) + + monkeypatch.setattr( + "controller.lib.mount_executor.subprocess.run", fake_run, + ) + executor = SubprocessMountExecutor() + result = executor.mount("UUID=1", "/media/x", "ext4", "defaults") + assert result.ok is False + assert "No such file" in result.error + def test_mount_propagates_stderr(self, monkeypatch): def fake_run(cmd, **kwargs): return _FakeCompletedProcess(returncode=32, stderr="busy") diff --git a/controller/test/test_reconciler.py b/controller/test/test_reconciler.py index 909b9ba5..f0d7263a 100644 --- a/controller/test/test_reconciler.py +++ b/controller/test/test_reconciler.py @@ -242,6 +242,17 @@ class TestUidGidInjection: assert "uid=2000" in opts assert "gid=2000" in opts + def test_injects_uid_gid_for_ntfs(self, tmp_path): + """ntfs/ntfs3 have no POSIX permissions either; secondary NTFS drives + must mount owned by the wrolpi user like exfat/vfat do.""" + fstab = FstabFile(mounts=[_entry(fstype="ntfs")]) + rec, fake = make_reconciler(tmp_path, fstab=fstab, + wrolpi_uid=2000, wrolpi_gid=2000) + rec.apply() + opts = fake.mount_calls[0].options + assert "uid=2000" in opts + assert "gid=2000" in opts + def test_does_not_double_inject_if_user_already_supplied(self, tmp_path): fstab = FstabFile(mounts=[_entry(fstype="vfat", options="defaults,uid=1234,gid=1234")])