From 23508fdc8957ec85e1e79ffc15fe0eb8eae0338b Mon Sep 17 00:00:00 2001 From: Sebastian Date: Thu, 10 Sep 2026 20:20:40 +0200 Subject: [PATCH] mount: fix iPod automount (lsblk flags + udisks suffixed paths) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs kept the connected iPod from being auto-mounted: - `lsblk -o PATH,LABEL -n -l -P` is invalid (--pairs and --list are mutually exclusive), so device lookup by label always failed with "no block device found" — a regression from the earlier parse fix. Drop -l. - udisks mounted the iPod volume at /media/sebastian/IPOD1 because the configured directory /media/sebastian/IPOD already exists, so even a resolved device failed with AlreadyMounted. The configured mount point is now treated as a hint: mounted_path_for() resolves an exact mount or the lowest-numbered udisks-suffixed sibling (IPOD → IPOD1) from /proc/self/mountinfo, and the sync destination, storage stats, settings display and fatsort resolution all use the real mount. Also harden fatsort: resolve_fatsort_target now refuses to run when the path is not a mount point, so findmnt can never fall back to the root filesystem's block device. Verified on the connected iPod: mounted_path_for → IPOD1, resolve_block_device → /dev/sdb2, mount_device → "already mounted at /media/sebastian/IPOD1", UI shows ● mounted with 9.7 GiB free · 92% and mount point ✓ /media/sebastian/IPOD1. Bump to 0.3.3. --- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 9 +++-- src/config.rs | 4 ++- src/disk.rs | 78 ++++++++++++++++++++++++++++++++++++++-- src/engine.rs | 62 +++++++++++++++++++++----------- src/udisks.rs | 62 ++++++++++++++++++++++++++++---- src/ui/tabs/dashboard.rs | 18 +++++++--- 8 files changed, 198 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dc52005..75aa44c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -253,7 +253,7 @@ dependencies = [ [[package]] name = "dap-tui" -version = "0.3.2" +version = "0.3.3" dependencies = [ "anyhow", "chrono", diff --git a/Cargo.toml b/Cargo.toml index fa462b5..903ad3e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "dap-tui" -version = "0.3.2" +version = "0.3.3" edition = "2021" description = "Device sync for portable players — TUI edition (ratatui)" license = "MIT" diff --git a/README.md b/README.md index 5ed0f92..e023175 100644 --- a/README.md +++ b/README.md @@ -107,9 +107,12 @@ mounts the target first (resolved by mount-point label via `lsblk`). Only the mlove `fatsort` step needs root (raw block device). Mount status and sync targets are validated against the kernel mount table -(`/proc/self/mountinfo`), not just directory existence: if the configured -`mount_point` exists but is not actually mounted, `dap-tui` refuses to sync to -it rather than writing the library onto the root filesystem. +(`/proc/self/mountinfo`), not just directory existence. The configured +`mount_point` is a hint: if udisks mounted the volume at a suffixed sibling +because the configured directory already existed (e.g. `IPOD` → `IPOD1`), +dap-tui follows the actual mount and uses it as the sync destination. If the +path exists but is not a mount at all, syncing into it is refused rather than +writing the library onto the root filesystem. ## Development diff --git a/src/config.rs b/src/config.rs index 950a941..dea63ac 100644 --- a/src/config.rs +++ b/src/config.rs @@ -122,12 +122,14 @@ pub struct Config { impl Config { /// Refresh `Device::mounted` from the kernel mount table (no subprocess). + /// A device counts as mounted when its volume is mounted at the configured + /// path or at a udisks-suffixed sibling (e.g. `IPOD` → `IPOD1`). pub fn refresh_mounts(&mut self) { for d in self.devices.iter_mut() { d.mounted = d .mount_point .as_deref() - .map(crate::disk::is_mounted) + .map(|p| crate::disk::mounted_path_for(p).is_some()) .unwrap_or(false); } } diff --git a/src/disk.rs b/src/disk.rs index 764c587..cffa509 100644 --- a/src/disk.rs +++ b/src/disk.rs @@ -1,4 +1,4 @@ -use std::path::Path; +use std::path::{Path, PathBuf}; /// Whether `path` is currently a mount point. /// @@ -26,6 +26,56 @@ pub fn is_mounted(path: &Path) -> bool { path.exists() } +/// Mount points currently listed in the kernel mount table. +fn mount_points() -> Vec { + #[cfg(target_os = "linux")] + if let Ok(text) = std::fs::read_to_string("/proc/self/mountinfo") { + return text + .lines() + .filter_map(|line| line.split(' ').nth(4)) + .map(|field| PathBuf::from(unescape_octal(field))) + .collect(); + } + Vec::new() +} + +/// Where the volume configured at `path` is actually mounted. +/// +/// udisks mounts a volume at `/media/$USER/$LABEL`; if that directory already +/// exists (e.g. a leftover mount dir after an unclean eject) it appends a +/// number, so `/media/sebastian/IPOD` becomes `/media/sebastian/IPOD1`. This +/// resolves the exact path or the lowest-numbered suffixed sibling, so the +/// configured path is treated as a hint rather than a hard requirement. +pub fn mounted_path_for(path: &Path) -> Option { + if is_mounted(path) { + return Some(path.to_path_buf()); + } + let parent = path.parent()?; + let base = path.file_name()?.to_string_lossy().to_string(); + mount_points() + .into_iter() + .filter_map(|mp| mount_suffix_rank(parent, &base, &mp).map(|rank| (rank, mp))) + .min_by_key(|(rank, _)| *rank) + .map(|(_, mp)| mp) +} + +/// Rank of a mount point relative to a configured path: 0 for an exact match, +/// `n` for `name{n}` (udisks' uniquifying suffix), `None` otherwise. +fn mount_suffix_rank(parent: &Path, base: &str, candidate: &Path) -> Option { + if candidate.parent() != Some(parent) { + return None; + } + let name = candidate.file_name()?.to_string_lossy(); + let rest = name.strip_prefix(base)?; + if rest.is_empty() { + return Some(0); + } + if rest.chars().all(|c| c.is_ascii_digit()) { + return rest.parse().ok(); + } + None +} + /// Undo the octal escaping (`\040` etc.) used by /proc mount tables. fn unescape_octal(s: &str) -> String { let bytes = s.as_bytes(); @@ -84,7 +134,31 @@ pub fn fmt_bytes(n: u64) -> String { #[cfg(test)] mod tests { - use super::{fmt_bytes, unescape_octal}; + use super::{fmt_bytes, mounted_path_for, mount_suffix_rank, unescape_octal}; + + #[test] + fn ranks_udisks_suffixed_mounts() { + let parent = std::path::Path::new("/media/sebastian"); + let rank = |c: &str| mount_suffix_rank(parent, "IPOD", std::path::Path::new(c)); + assert_eq!(rank("/media/sebastian/IPOD"), Some(0)); + assert_eq!(rank("/media/sebastian/IPOD1"), Some(1)); + assert_eq!(rank("/media/sebastian/IPOD12"), Some(12)); + // non-numeric suffixes and other directories must not match + assert_eq!(rank("/media/sebastian/IPODX"), None); + assert_eq!(rank("/media/sebastian/IPOD-2"), None); + assert_eq!(rank("/media/other/IPOD1"), None); + assert_eq!(rank("/media/sebastian/IPOD1/extra"), None); + } + + #[test] + #[cfg(target_os = "linux")] + fn resolves_exact_mounts() { + // `/` is always mounted; mounted_path_for must return it unchanged. + assert_eq!( + mounted_path_for(std::path::Path::new("/")), + Some(std::path::PathBuf::from("/")) + ); + } #[test] fn unescapes_mountinfo_paths() { diff --git a/src/engine.rs b/src/engine.rs index eb195d5..b1dea7f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -542,29 +542,43 @@ fn scan_music_dir(base: &Path) -> HashMap { } fn run_rsync_device(config: &Config, device: &Device, ctx: &WorkerCtx) -> Result<(), String> { - let mount = device + let configured = device .mount_point .as_ref() .ok_or_else(|| "mount_point is not set for this device".to_string())?; - if !crate::disk::is_mounted(mount) { - ctx.log( - LogLevel::Info, - format!("device not mounted at {} — trying udisksctl…", mount.display()), - ); - match crate::udisks::mount_device(mount) { - Ok(msg) => ctx.log(LogLevel::Info, format!("auto-mounted: {msg}")), - Err(e) => { - return Err(format!( - "device not mounted at {} — connect & mount it (m) and retry. {e}", - mount.display() - )); + // Trust the kernel mount table over the configured path: udisks may have + // mounted the volume at a suffixed path (e.g. /media/.../IPOD1) because the + // configured directory already existed. + let mount = match crate::disk::mounted_path_for(configured) { + Some(p) => p, + None => { + ctx.log( + LogLevel::Info, + format!( + "device not mounted at {} — trying udisksctl…", + configured.display() + ), + ); + match crate::udisks::mount_device(configured) { + Ok(msg) => ctx.log(LogLevel::Info, format!("auto-mounted: {msg}")), + Err(e) => { + return Err(format!( + "device not mounted at {} — connect & mount it (m) and retry. {e}", + configured.display() + )); + } } + crate::disk::mounted_path_for(configured).ok_or_else(|| { + format!( + "auto-mount of {} did not yield a mount point", + configured.display() + ) + })? } - } - // A leftover (empty) mount-point directory must not be mistaken for a - // mounted device — rsync would otherwise happily fill the root filesystem. - if !crate::disk::is_mounted(mount) { + }; + // Paranoia: never write into a directory that is not actually a mount. + if !crate::disk::is_mounted(&mount) { return Err(format!( "{} exists but is not a mount point — refusing to sync. Connect the device and press 'm'.", mount.display() @@ -732,15 +746,17 @@ fn capture(cmd: &str, args: &[&str]) -> Result<(i32, String), String> { } fn finalize_fat(device: &Device, ctx: &WorkerCtx) -> Result<(), String> { - let mount = device + let configured = device .mount_point .as_ref() .ok_or_else(|| "mount_point is required to run fatsort".to_string())?; + // Use the actual mount point in case udisks moved it to a suffixed path. + let mount = crate::disk::mounted_path_for(configured).unwrap_or_else(|| configured.clone()); - let target = resolve_fatsort_target(mount)?; + let target = resolve_fatsort_target(&mount)?; ctx.log(LogLevel::Info, format!("Unmounting {mount_display}", mount_display = mount.display())); - match crate::udisks::unmount_device(mount) { + match crate::udisks::unmount_device(&mount) { Ok(msg) => ctx.log(LogLevel::Info, format!("unmounted: {msg}")), Err(e) => { // fall back to sudo for setups without udisks @@ -771,6 +787,12 @@ fn resolve_fatsort_target(mount: &Path) -> Result { if !mount.is_dir() { return Ok(mount.display().to_string()); } + // Never let findmnt fall back to the parent filesystem (e.g. the root + // disk) when the directory is not actually a mount point — fatsort on the + // wrong block device would corrupt it. + if !crate::disk::is_mounted(mount) { + return Err(format!("{} is not a mount point", mount.display())); + } let (code, out) = capture( "findmnt", &["-n", "-o", "SOURCE", "-T", mount.to_str().unwrap_or("")], diff --git a/src/udisks.rs b/src/udisks.rs index 026f468..646786e 100644 --- a/src/udisks.rs +++ b/src/udisks.rs @@ -19,11 +19,10 @@ fn run(cmd: &str, args: &[&str]) -> (bool, String) { } fn resolve_block_device(path: &Path) -> Option { - // Only trust findmnt when the path really is a mount point; otherwise - // findmnt happily reports the containing filesystem (e.g. /), which would - // make us "mount" the root device. - if crate::disk::is_mounted(path) { - let (ok, out) = run("findmnt", &["-n", "-o", "SOURCE", "-T", path.to_str()?]); + // If the volume is already mounted (possibly at a udisks-suffixed path + // like IPOD1), ask findmnt for its source device. + if let Some(mp) = crate::disk::mounted_path_for(path) { + let (ok, out) = run("findmnt", &["-n", "-o", "SOURCE", "-T", mp.to_str()?]); if ok { let src = out.trim(); if src.starts_with("/dev/") { @@ -31,8 +30,10 @@ fn resolve_block_device(path: &Path) -> Option { } } } + // Not mounted: match a partition label to the configured mount point's + // basename (e.g. `/media/sebastian/IPOD` → label `IPOD`). let wanted = path.file_name()?.to_string_lossy().to_lowercase(); - let (ok, out) = run("lsblk", &["-o", "PATH,LABEL", "-n", "-l", "-P"]); + let (ok, out) = run("lsblk", &["-o", "PATH,LABEL", "-n", "-P"]); if !ok { return None; } @@ -50,6 +51,16 @@ fn resolve_block_device(path: &Path) -> Option { None } +/// Current mount target of a block device, if any. +fn mounted_target(dev: &str) -> Option { + let (ok, out) = run("findmnt", &["-n", "-o", "TARGET", "-S", dev]); + if ok && !out.trim().is_empty() { + Some(std::path::PathBuf::from(out.trim())) + } else { + None + } +} + /// Extract a `KEY="value"` field from `lsblk -P` output. fn field(line: &str, key: &str) -> Option { let needle = format!("{key}=\""); @@ -60,13 +71,25 @@ fn field(line: &str, key: &str) -> Option { } pub fn mount_device(path: &Path) -> Result { + // Already mounted, possibly at a udisks-suffixed path. + if let Some(mp) = crate::disk::mounted_path_for(path) { + return Ok(format!("already mounted at {}", mp.display())); + } let Some(dev) = resolve_block_device(path) else { return Err(format!("no block device found for {}", path.display())); }; + if let Some(mp) = mounted_target(&dev) { + return Ok(format!("already mounted at {}", mp.display())); + } let (ok, detail) = run("udisksctl", &["mount", "-b", dev.as_str()]); if ok { Ok(detail) } else { + // A concurrent/late mount makes AlreadyMounted spurious: report + // success when the kernel now has the device mounted. + if let Some(mp) = mounted_target(&dev) { + return Ok(format!("mounted at {}", mp.display())); + } Err(format!("udisksctl mount {dev} failed: {detail}")) } } @@ -82,3 +105,30 @@ pub fn unmount_device(path: &Path) -> Result { Err(format!("udisksctl unmount {dev} failed: {detail}")) } } + +#[cfg(test)] +mod tests { + use super::field; + + #[test] + fn parses_lsblk_pairs() { + assert_eq!( + field(r#"PATH="/dev/sdb2" LABEL="IPOD""#, "PATH").as_deref(), + Some("/dev/sdb2") + ); + assert_eq!( + field(r#"PATH="/dev/sdb2" LABEL="IPOD""#, "LABEL").as_deref(), + Some("IPOD") + ); + assert_eq!( + field(r#"PATH="/dev/sdb1" LABEL="""#, "LABEL").as_deref(), + Some("") + ); + assert_eq!( + field(r#"LABEL="My Disk" PATH="/dev/sdc1""#, "LABEL").as_deref(), + Some("My Disk") + ); + assert_eq!(field(r#"PATH="/dev/sdc1""#, "LABEL"), None); + } + +} diff --git a/src/ui/tabs/dashboard.rs b/src/ui/tabs/dashboard.rs index 69775b3..6a1e9ec 100644 --- a/src/ui/tabs/dashboard.rs +++ b/src/ui/tabs/dashboard.rs @@ -147,11 +147,14 @@ fn draw_device_card(f: &mut Frame, area: Rect, app: &App, device: &crate::config line("last sync", last, last_style), ]; - // Mounted storage usage. The compact value ("276.8 GiB free · 39%") fits - // the card width, unlike the old "459.5 GiB total · 276.8 GiB free" which - // was truncated mid-number. + // Mounted storage usage. udisks may mount the volume at a suffixed path + // (IPOD → IPOD1), so report stats for wherever it actually is mounted. + let actual_mount = device + .mount_point + .as_deref() + .and_then(crate::disk::mounted_path_for); let storage = if device.mounted { - device.mount_point.as_ref().and_then(|p| disk_stats(p)) + actual_mount.as_deref().and_then(disk_stats) } else { None }; @@ -232,7 +235,12 @@ fn draw_settings_card(f: &mut Frame, area: Rect, device: &crate::config::Device) Style::default().fg(theme::FG), )); match device.mount_point.as_deref() { - Some(p) => lines.push(path_kv(l_mount, p, value_w, lw)), + Some(p) => { + // Show where it is actually mounted when udisks used a suffixed + // path (e.g. the configured IPOD dir existed, so it chose IPOD1). + let shown = crate::disk::mounted_path_for(p).unwrap_or_else(|| p.to_path_buf()); + lines.push(path_kv(l_mount, &shown, value_w, lw)); + } None => lines.push(line(l_mount, "—".to_string(), Style::default().fg(theme::DIM))), } lines.push(line(