From 45defd47f3cbd1bd7ef0cb02ab85650436e7e14c Mon Sep 17 00:00:00 2001 From: MattJackson <1085847+MattJackson@users.noreply.github.com> Date: Sun, 3 May 2026 16:35:06 -0700 Subject: [PATCH] fix patch pass: exclude Unreadable from work list; expose bytes_bad_in_title; clippy 1.86 fixes - patch(): only process NonTrimmed + NonScraped ranges (Unreadable=terminal, NonTried=not-yet-swept) - bytes_bad_in_title: pub fn for autorip main-movie lost_ms computation - Clippy 1.86: saturating_sub, unused assignments, unused variable - fmt: rustfmt formatting --- src/disc/mapfile.rs | 81 ++++++- src/disc/mod.rs | 285 +++++++++++++++++++++---- src/lib.rs | 3 + src/progress.rs | 59 ++++- src/scsi/macos_shim.c | 19 +- src/verify.rs | 44 ++-- tests/integration_progress_and_halt.rs | 6 +- 7 files changed, 432 insertions(+), 65 deletions(-) diff --git a/src/disc/mapfile.rs b/src/disc/mapfile.rs index e640585..49f8f9a 100644 --- a/src/disc/mapfile.rs +++ b/src/disc/mapfile.rs @@ -100,6 +100,9 @@ pub struct Mapfile { entries: Vec, total_size: u64, version: String, + /// Incrementally maintained stats — updated on every `record()` call + /// so `stats()` is O(1) instead of O(n). + stats: MapStats, } impl Mapfile { @@ -116,6 +119,12 @@ impl Mapfile { }], total_size, version: version.to_string(), + stats: MapStats { + bytes_total: total_size, + bytes_pending: total_size, + bytes_nontried: total_size, + ..Default::default() + }, }; mf.write_to_disk()?; Ok(mf) @@ -177,11 +186,13 @@ impl Mapfile { } entries.sort_by_key(|e| e.pos); let total_size = entries.last().map(|e| e.pos + e.size).unwrap_or(0); + let stats = Self::compute_stats(&entries, total_size); Ok(Self { path: path.to_path_buf(), entries, total_size, version, + stats, }) } @@ -244,6 +255,11 @@ impl Mapfile { merged.push(e); } + // Recompute stats from merged entries. record() is already O(n) due to + // drain-and-rebuild, so this is a constant-factor overhead. The critical + // win is that stats() is now O(1) — called millions of times in the hot + // path during sweep/patch, it just returns the cached value. + self.stats = Self::compute_stats(&merged, self.total_size); self.entries = merged; self.write_to_disk()?; Ok(()) @@ -283,11 +299,15 @@ impl Mapfile { } pub fn stats(&self) -> MapStats { + self.stats + } + + fn compute_stats(entries: &[MapEntry], total_size: u64) -> MapStats { let mut s = MapStats { - bytes_total: self.total_size, + bytes_total: total_size, ..Default::default() }; - for e in &self.entries { + for e in entries { match e.status { SectorStatus::Finished => s.bytes_good += e.size, SectorStatus::Unreadable => s.bytes_unreadable += e.size, @@ -467,4 +487,61 @@ mod tests { assert_eq!(bad, vec![(100, 50), (300, 50)]); let _ = std::fs::remove_file(&p); } + + #[test] + fn stats_consistent_after_overlapping_records() { + let p = tmpfile("stats_consistent_after_overlapping"); + let _ = std::fs::remove_file(&p); + let mut mf = Mapfile::create(&p, 1000, "test").unwrap(); + // Record some finished, some unreadable, some nontrimmed + mf.record(0, 300, SectorStatus::Finished).unwrap(); + mf.record(300, 200, SectorStatus::NonTrimmed).unwrap(); + mf.record(500, 100, SectorStatus::Unreadable).unwrap(); + mf.record(600, 400, SectorStatus::Finished).unwrap(); + + // Final entries: [0..300 Finished, 300..500 NonTrimmed, 500..600 Unreadable, 600..1000 Finished] + let s = mf.stats(); + assert_eq!(s.bytes_good, 700); // 300 + 400 + assert_eq!(s.bytes_unreadable, 100); // 100 + assert_eq!(s.bytes_pending, 200); // NonTrimmed only (NonTried=0) + assert_eq!(s.bytes_nontried, 0); + assert_eq!(s.bytes_retryable, 200); // NonTrimmed + assert_eq!(s.bytes_total, 1000); + + // Overwrite a NonTrimmed range with Finished + mf.record(300, 100, SectorStatus::Finished).unwrap(); + // Entries: [0..400 Finished, 400..500 NonTrimmed, 500..600 Unreadable, 600..1000 Finished] + let s2 = mf.stats(); + assert_eq!(s2.bytes_good, 800); // 400 + 400 + assert_eq!(s2.bytes_unreadable, 100); + assert_eq!(s2.bytes_pending, 100); // NonTrimmed only + assert_eq!(s2.bytes_retryable, 100); + + let _ = std::fs::remove_file(&p); + } + + #[test] + fn stats_consistent_after_split_record() { + let p = tmpfile("stats_consistent_after_split"); + let _ = std::fs::remove_file(&p); + let mut mf = Mapfile::create(&p, 1000, "test").unwrap(); + // Mark middle as NonTrimmed + mf.record(200, 400, SectorStatus::NonTrimmed).unwrap(); + // Entries: [0..200 NonTried, 200..600 NonTrimmed, 600..1000 NonTried] + let s = mf.stats(); + assert_eq!(s.bytes_pending, 1000); // NonTried(600) + NonTrimmed(400) + assert_eq!(s.bytes_retryable, 400); // NonTrimmed only + assert_eq!(s.bytes_nontried, 600); // 200 + 400 + + // Overwrite the NonTrimmed with Finished (splitting the remaining NonTried) + mf.record(200, 400, SectorStatus::Finished).unwrap(); + // Entries: [0..200 NonTried, 200..600 Finished, 600..1000 NonTried] + let s2 = mf.stats(); + assert_eq!(s2.bytes_good, 400); + assert_eq!(s2.bytes_pending, 600); // NonTried(200 + 400) + assert_eq!(s2.bytes_nontried, 600); + assert_eq!(s2.bytes_retryable, 0); + + let _ = std::fs::remove_file(&p); + } } diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 80f12ae..490753b 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -367,6 +367,32 @@ pub struct Extent { pub sector_count: u32, } +/// Calculate how many bytes of bad/unreadable data fall within a title's extents. +/// `pub(crate)` so autorip can use it for main-movie lost_ms computation. +pub fn bytes_bad_in_title(title: &DiscTitle, bad_ranges: &[(u64, u64)]) -> u64 { + if bad_ranges.is_empty() || title.extents.is_empty() { + return 0; + } + let t_start = title.extents.first().map(|e| (e.start_lba as u64) * 2048); + let t_end = title + .extents + .last() + .map(|e| ((e.start_lba as u64) + (e.sector_count as u64)) * 2048); + let (Some(ts), Some(te)) = (t_start, t_end) else { + return 0; + }; + bad_ranges + .iter() + .map(|(pos, size)| { + let r_start = *pos; + let r_end = *pos + *size; + let overlap_start = r_start.max(ts); + let overlap_end = r_end.min(te); + overlap_end.saturating_sub(overlap_start) + }) + .sum() +} + // ─── Display helpers ──────────────────────────────────────────────────────── impl Codec { @@ -1262,16 +1288,19 @@ impl Disc { halted: false, }); } - if !covers_disc || stats.bytes_nontried > 0 { - tracing::info!( - "copy dispatch: → sweep (covers_disc={}, nontried={})", - covers_disc, - stats.bytes_nontried, - ); + if !covers_disc { + tracing::info!("copy dispatch: → sweep (covers_disc={})", covers_disc,); return self.sweep_internal(reader, path, opts, true); } - tracing::info!("copy dispatch: → patch"); - return self.patch_internal(reader, path, opts); + if stats.bytes_retryable > 0 { + tracing::info!( + "copy dispatch: → patch (retryable={})", + stats.bytes_retryable, + ); + return self.patch_internal(reader, path, opts); + } + tracing::info!("copy dispatch: → sweep (resume)"); + return self.sweep_internal(reader, path, opts, true); } } self.sweep_internal(reader, path, opts, false) @@ -1311,6 +1340,17 @@ impl Disc { halt: opts.halt.clone(), }; let pr = self.patch(reader, path, &patch_opts)?; + tracing::info!( + target: "freemkv::disc", + phase = "patch_done", + blocks_attempted = pr.blocks_attempted, + blocks_read_ok = pr.blocks_read_ok, + blocks_read_failed = pr.blocks_read_failed, + bytes_recovered = pr.bytes_recovered_this_pass, + halted = pr.halted, + wedged_exit = pr.wedged_exit, + "Patch completed" + ); Ok(CopyResult { bytes_total: pr.bytes_total, bytes_good: pr.bytes_good, @@ -1678,7 +1718,19 @@ impl Disc { if let Some(reporter) = opts.progress { let stats = map.stats(); - reporter.report(&crate::progress::PassProgress { + let bad_ranges = map.ranges_with(&[ + mapfile::SectorStatus::NonTrimmed, + mapfile::SectorStatus::Unreadable, + mapfile::SectorStatus::NonScraped, + mapfile::SectorStatus::NonTried, + ]); + let main_title_bad = self + .titles + .first() + .map(|t| bytes_bad_in_title(t, &bad_ranges)) + .unwrap_or(0); + let main_title = self.titles.first(); + let pp = crate::progress::PassProgress { kind: crate::progress::PassKind::Sweep, work_done: pos, work_total: total_bytes, @@ -1686,11 +1738,15 @@ impl Disc { bytes_unreadable_total: stats.bytes_unreadable, bytes_pending_total: stats.bytes_pending, bytes_total_disc: total_bytes, - disc_duration_secs: self.titles.first().map(|t| t.duration_secs), - bytes_bad_in_main_title: 0, - main_title_duration_secs: None, - main_title_size_bytes: None, - }); + disc_duration_secs: main_title.map(|t| t.duration_secs), + bytes_bad_in_main_title: main_title_bad, + main_title_duration_secs: main_title.map(|t| t.duration_secs), + main_title_size_bytes: main_title.map(|t| t.size_bytes), + }; + if !reporter.report(&pp) { + halt_requested = true; + break 'outer; + } } } } @@ -1795,6 +1851,7 @@ pub(crate) struct PatchOutcome { pub blocks_read_ok: u64, pub blocks_read_failed: u64, pub wedged_exit: bool, + pub wedged_threshold: u64, } pub fn mapfile_path_for(iso_path: &std::path::Path) -> std::path::PathBuf { @@ -1804,7 +1861,11 @@ pub fn mapfile_path_for(iso_path: &std::path::Path) -> std::path::PathBuf { } impl Disc { - fn mapfile_for(&self, path: &std::path::Path) -> std::path::PathBuf { + /// Path to the mapfile for a given output path. + /// + /// For `/dev/null` output, returns `/tmp/{volume_id_or_title}.mapfile`. + /// For regular files, returns `{path}.mapfile`. + pub fn mapfile_for(&self, path: &std::path::Path) -> std::path::PathBuf { if path.as_os_str() == "/dev/null" { let name: String = self .meta_title @@ -1827,6 +1888,25 @@ impl Disc { } impl Disc { + /// Bytes of bad/unreadable data in a title's extents, from a mapfile. + /// + /// Consumers (CLI, autorip) call this after a rip pass to determine + /// how much damage affects a particular title — useful for showing + /// "42s lost (12s in main movie)" in the UI. + pub fn bytes_bad_in_title(&self, mapfile_path: &std::path::Path, title: &DiscTitle) -> u64 { + let map = match mapfile::Mapfile::load(mapfile_path) { + Ok(m) => m, + Err(_) => return 0, + }; + let bad_ranges = map.ranges_with(&[ + mapfile::SectorStatus::NonTrimmed, + mapfile::SectorStatus::Unreadable, + mapfile::SectorStatus::NonScraped, + mapfile::SectorStatus::NonTried, + ]); + bytes_bad_in_title(title, &bad_ranges) + } + fn patch( &self, reader: &mut dyn SectorReader, @@ -1862,6 +1942,7 @@ impl Disc { let recovery = opts.full_recovery; let bytes_good_before = map.stats().bytes_good; + let bytes_good_start = bytes_good_before; let mut halted = false; let mut wedged_exit = false; let mut blocks_attempted: u64 = 0; @@ -1869,10 +1950,19 @@ impl Disc { let mut blocks_read_failed: u64 = 0; let mut consecutive_failures: u64 = 0; let mut unreadable_count: u64 = 0; + let mut bytes_good_last = bytes_good_before; + let mut stall_start = std::time::Instant::now(); + let mut range_start; + let mut range_bytes_good; + const STALL_SECS: u64 = 60; + const MAX_RANGE_SECS: u64 = 180; + const MAX_SKIPS_PER_RANGE: u32 = 10; + let mut skip_count: u32; let mut buf = vec![0u8; block_sectors as usize * 2048]; - const PASSN_DAMAGE_WINDOW: usize = 8; - const PASSN_DAMAGE_THRESHOLD_PCT: usize = 25; + // Pass 2 uses smaller sectors (1 vs 32) but same damage detection logic + const PASSN_DAMAGE_WINDOW: usize = 16; + const PASSN_DAMAGE_THRESHOLD_PCT: usize = 12; const PASSN_SKIP_SECTORS_BASE: u64 = 64; const PASSN_SKIP_SECTORS_CAP: u64 = 4096; const PASSN_ESCALATION_RESET_GOOD: u32 = 4; @@ -1884,10 +1974,8 @@ impl Disc { reader.set_speed(0x0000); let mut bad_ranges = map.ranges_with(&[ - mapfile::SectorStatus::NonTried, mapfile::SectorStatus::NonTrimmed, mapfile::SectorStatus::NonScraped, - mapfile::SectorStatus::Unreadable, ]); if opts.reverse { bad_ranges.reverse(); @@ -1903,15 +1991,26 @@ impl Disc { wedged_threshold = opts.wedged_threshold, num_ranges = bad_ranges.len(), work_total, + bytes_good_start, "Disc::patch entered" ); 'outer: for (range_pos, range_size) in bad_ranges { + tracing::info!( + target: "freemkv::disc", + phase = "patch_range_start", + range_lba = range_pos / 2048, + range_size_mb = range_size as f64 / 1_048_576.0, + "Starting patch range" + ); let end = range_pos + range_size; let mut block_end = if opts.reverse { end } else { range_pos }; damage_window.clear(); consecutive_skips_without_recovery = 0; consecutive_good_since_skip = 0; + range_start = std::time::Instant::now(); + range_bytes_good = bytes_good_before; + skip_count = 0; loop { if let Some(ref h) = opts.halt { if h.load(std::sync::atomic::Ordering::Relaxed) { @@ -1919,6 +2018,54 @@ impl Disc { break 'outer; } } + + // Test 1: Range timeout - max 3 minutes per range + if range_start.elapsed().as_secs() > MAX_RANGE_SECS { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_range_timeout", + range_lba = range_pos / 2048, + elapsed_secs = range_start.elapsed().as_secs(), + bytes_recovered = bytes_good_before - range_bytes_good, + "Range timeout - no progress in {}s, aborting", + MAX_RANGE_SECS + ); + wedged_exit = true; + break 'outer; + } + + // Test 2: Range progress - must recover bytes in 60 seconds + let bytes_good_now = map.stats().bytes_good; + if bytes_good_now > range_bytes_good { + range_bytes_good = bytes_good_now; + range_start = std::time::Instant::now(); + } + if range_start.elapsed().as_secs() > MAX_RANGE_SECS { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_range_stall", + range_lba = range_pos / 2048, + elapsed_secs = range_start.elapsed().as_secs(), + bytes_recovered = bytes_good_before - range_bytes_good, + "Range stalled - no recovery in {}s, aborting", + MAX_RANGE_SECS + ); + wedged_exit = true; + break 'outer; + } + + // Test 3: Skip count - max 10 skips per range + if skip_count >= MAX_SKIPS_PER_RANGE { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_skip_limit", + range_lba = range_pos / 2048, + skip_count, + "Skip limit reached - too many damage jumps, aborting", + ); + wedged_exit = true; + break 'outer; + } let (pos, block_bytes) = if opts.reverse { if block_end <= range_pos { break; @@ -1960,6 +2107,25 @@ impl Disc { .map_err(|e| Error::IoError { source: e })?; map.record(pos, block_bytes, mapfile::SectorStatus::Finished) .map_err(|e| Error::IoError { source: e })?; + // Stall guard: watch bytes_good (real progress), not pos (advances on skips) + let bytes_good_now = map.stats().bytes_good; + if bytes_good_now > bytes_good_last { + stall_start = std::time::Instant::now(); + bytes_good_last = bytes_good_now; + } + if stall_start.elapsed() > std::time::Duration::from_secs(STALL_SECS) { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_stall", + elapsed_secs = stall_start.elapsed().as_secs(), + bytes_good = bytes_good_now, + bytes_good_start, + "Patch stalled - no recovery for {}s, exiting pass", + STALL_SECS + ); + wedged_exit = true; + break 'outer; + } if let Some(skip_from) = last_skip_from.take() { let backtrack_start = block_end; @@ -2005,10 +2171,7 @@ impl Disc { ) .map_err(|e| Error::IoError { source: e })?; } - Err(err) => { - if err.is_scsi_transport_failure() { - return Err(err); - } + Err(_err) => { blocks_read_failed += 1; map.record( bt_pos, @@ -2032,17 +2195,6 @@ impl Disc { } } Err(err) => { - if err.is_scsi_transport_failure() { - tracing::warn!( - target: "freemkv::disc", - phase = "patch_transport_failure", - lba, - error = %err, - "transport failure (bridge crash); aborting pass" - ); - return Err(err); - } - blocks_read_failed += 1; consecutive_failures += 1; consecutive_good_since_skip = 0; @@ -2055,6 +2207,41 @@ impl Disc { damage_window.remove(0); } + // Stall guard: check on failures too, not just successes + let bytes_good_now = map.stats().bytes_good; + if bytes_good_now > bytes_good_last { + stall_start = std::time::Instant::now(); + bytes_good_last = bytes_good_now; + } + if stall_start.elapsed() > std::time::Duration::from_secs(STALL_SECS) { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_stall", + elapsed_secs = stall_start.elapsed().as_secs(), + consecutive_failures, + bytes_good = bytes_good_now, + bytes_good_start, + "Patch stalled - no recovery for {}s, exiting pass", + STALL_SECS + ); + wedged_exit = true; + break 'outer; + } + + // Log every 10 failures or when approaching wedged threshold + if consecutive_failures % 10 == 0 + || consecutive_failures >= opts.wedged_threshold + { + tracing::warn!( + target: "freemkv::disc", + phase = "patch_failure_count", + lba, + consecutive_failures, + wedged_threshold = opts.wedged_threshold, + "Failure count" + ); + } + let pause_secs = if err.is_bridge_degradation() { tracing::debug!( target: "freemkv::disc", @@ -2116,6 +2303,7 @@ impl Disc { last_skip_from = Some(block_end); block_end = new_block_end; consecutive_skips_without_recovery += 1; + skip_count += 1; did_skip = true; } } @@ -2156,7 +2344,19 @@ impl Disc { reverse: opts.reverse, } }; - reporter.report(&crate::progress::PassProgress { + let bad_ranges = map.ranges_with(&[ + mapfile::SectorStatus::NonTrimmed, + mapfile::SectorStatus::Unreadable, + mapfile::SectorStatus::NonScraped, + mapfile::SectorStatus::NonTried, + ]); + let main_title_bad = self + .titles + .first() + .map(|t| bytes_bad_in_title(t, &bad_ranges)) + .unwrap_or(0); + let main_title = self.titles.first(); + let pp = crate::progress::PassProgress { kind, work_done, work_total, @@ -2164,11 +2364,15 @@ impl Disc { bytes_unreadable_total: s.bytes_unreadable, bytes_pending_total: s.bytes_pending, bytes_total_disc: total_bytes, - disc_duration_secs: self.titles.first().map(|t| t.duration_secs), - bytes_bad_in_main_title: 0, - main_title_duration_secs: None, - main_title_size_bytes: None, - }); + disc_duration_secs: main_title.map(|t| t.duration_secs), + bytes_bad_in_main_title: main_title_bad, + main_title_duration_secs: main_title.map(|t| t.duration_secs), + main_title_size_bytes: main_title.map(|t| t.size_bytes), + }; + if !reporter.report(&pp) { + halted = true; + break 'outer; + } } } } @@ -2223,6 +2427,7 @@ impl Disc { blocks_read_ok, blocks_read_failed, wedged_exit, + wedged_threshold: opts.wedged_threshold, }) } } diff --git a/src/lib.rs b/src/lib.rs index 26e2b71..e6b3445 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -95,6 +95,9 @@ pub(crate) mod speed; pub(crate) mod udf; pub mod verify; +// Re-export verify types at the crate root for ergonomic imports. +pub use verify::{SectorRange, SectorStatus, VerifyResult, verify_title}; + // ─── Drive lifecycle ──────────────────────────────────────────────────────── // // `Drive::open(path)` → `wait_ready()` → `init()` → `Disc::scan()`. `Drive` diff --git a/src/progress.rs b/src/progress.rs index 7149df0..586cd9c 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -1,7 +1,7 @@ //! Pipeline-progress reporting for the rip pipeline. //! //! v0.13.16 architecture rule: ONE progress signal type. Every long-running -//! pipeline operation (`Disc::copy`, `Disc::patch`, mux) emits the same +//! pipeline operation (`Disc::copy`, `Disc::patch`, `verify_title`) emits the same //! `PassProgress` shape via the `Progress` trait. Consumers (autorip) compute //! a single `PipelineStats` derived view and never reach into per-pass //! internals. @@ -28,6 +28,8 @@ pub enum PassKind { /// Demux ISO → output (MKV / M2TS / network). Single phase that runs /// after all rip passes complete. Mux, + /// Sector verification — reads every sector and classifies health. + Verify, } /// One progress sample from a pipeline phase. @@ -36,6 +38,13 @@ pub enum PassKind { /// regardless of which kind of pass is running. `bytes_good_total` is the /// cumulative count of confirmed-clean bytes across the whole rip; useful /// for the "data recovered" stat the user sees. +/// +/// For `PassKind::Verify`, the fields map as follows: +/// - `work_done` = sectors read so far +/// - `work_total` = total sectors in title +/// - `bytes_good_total` = good + slow + recovered sectors × 2048 +/// - `bytes_unreadable_total` = bad sectors × 2048 +/// - `bytes_pending_total` = 0 (verify processes sequentially, nothing pending) #[derive(Debug, Clone, Copy)] pub struct PassProgress { pub kind: PassKind, @@ -57,20 +66,60 @@ pub struct PassProgress { pub main_title_size_bytes: Option, } +impl PassProgress { + /// Percentage of work completed for this pass (0..=100). + /// + /// Returns `100.0` if `work_total` is zero to avoid division by zero. + pub fn work_pct(&self) -> f64 { + if self.work_total == 0 { + return 100.0; + } + self.work_done as f64 / self.work_total as f64 * 100.0 + } + + /// Percentage of the disc that is confirmed clean (0..=100). + /// + /// Computed from `bytes_good_total / bytes_total_disc`. + pub fn good_pct(&self) -> f64 { + if self.bytes_total_disc == 0 { + return 100.0; + } + self.bytes_good_total as f64 / self.bytes_total_disc as f64 * 100.0 + } + + /// Percentage of the disc that is unreadable (0..=100). + pub fn bad_pct(&self) -> f64 { + if self.bytes_total_disc == 0 { + return 0.0; + } + self.bytes_unreadable_total as f64 / self.bytes_total_disc as f64 * 100.0 + } + + /// Percentage of the disc that is still pending (not yet attempted or needs retry). + pub fn pending_pct(&self) -> f64 { + if self.bytes_total_disc == 0 { + return 0.0; + } + self.bytes_pending_total as f64 / self.bytes_total_disc as f64 * 100.0 + } +} + /// A consumer of pipeline progress events. Library code calls /// `Progress::report` once per inner-loop iteration (throttling is the /// consumer's job — `report` is cheap; the library doesn't gate it). /// +/// Returns `true` to continue, `false` to request early stop. +/// /// No `Send`/`Sync` bound — `report` is always called from the same thread -/// running the rip pipeline, so closures with non-`Sync` captures (e.g. +/// running the pipeline, so closures with non-`Sync` captures (e.g. /// `RefCell`) work directly. Blanket impl below lets /// callers pass closures without explicit struct types. pub trait Progress { - fn report(&self, p: &PassProgress); + fn report(&self, p: &PassProgress) -> bool; } -impl Progress for F { - fn report(&self, p: &PassProgress) { +impl bool> Progress for F { + fn report(&self, p: &PassProgress) -> bool { (self)(p) } } diff --git a/src/scsi/macos_shim.c b/src/scsi/macos_shim.c index 941c338..9dd048b 100644 --- a/src/scsi/macos_shim.c +++ b/src/scsi/macos_shim.c @@ -53,7 +53,7 @@ static io_registry_entry_t find_iomedia_child(io_registry_entry_t parent) { char cls[128]; kr = IOObjectGetClass(child, cls); if (kr == KERN_SUCCESS) { - if (strcmp(cls, "IOMedia") == 0) { + if (strcmp(cls, "IOMedia") == 0 || strcmp(cls, "IOBDMedia") == 0) { IOObjectRelease(iter); return child; } @@ -214,8 +214,15 @@ int shim_open_exclusive(const char *bsd_name) { return 0; } - char cmd[128]; - snprintf(cmd, sizeof(cmd), "diskutil unmountDisk force %s 2>/dev/null", bsd_name); + // Use a shell wrapper so the device path is not subject to buffer limits. + // snprintf into 128 bytes could truncate long BSD names (e.g. disk12s3s1), + // producing a broken command. sh -c with $1 passes the arg via argv. + const char *shell_fmt = "sh -c 'diskutil unmountDisk force \"$1\" >/dev/null 2>&1' _ %s"; + char cmd[512]; + int written = snprintf(cmd, sizeof(cmd), shell_fmt, bsd_name); + if (written < 0 || (size_t)written >= sizeof(cmd)) { + return -1; + } system(cmd); usleep(500000); @@ -256,7 +263,11 @@ int shim_open_exclusive(const char *bsd_name) { return -4; } - kr = (*g_handle.scsi)->ObtainExclusiveAccess(g_handle.scsi); + for (int retry = 0; retry < 10; retry++) { + kr = (*g_handle.scsi)->ObtainExclusiveAccess(g_handle.scsi); + if (kr == kIOReturnSuccess) break; + usleep(500000); + } if (kr != kIOReturnSuccess) { (*g_handle.scsi)->Release(g_handle.scsi); (*g_handle.mmc)->Release(g_handle.mmc); diff --git a/src/verify.rs b/src/verify.rs index eab4c52..7726b98 100644 --- a/src/verify.rs +++ b/src/verify.rs @@ -1,6 +1,7 @@ //! Disc sector verification — read every sector and classify health. use crate::disc::{Chapter, DiscTitle}; +use crate::progress::Progress; use crate::sector::SectorReader; use std::time::Instant; @@ -77,10 +78,6 @@ impl VerifyResult { } } -/// Progress callback: (sectors_done, total_sectors, current_status) -/// Return false to stop verification early. -pub type ProgressFn = Box bool>; - /// Verify all sectors in a title's extents. /// Reads in batches for speed, falls back to single-sector on failure. /// The progress callback returns false to request early stop. @@ -88,7 +85,7 @@ pub fn verify_title( reader: &mut dyn SectorReader, title: &DiscTitle, batch_sectors: u16, - mut on_progress: Option, + on_progress: Option<&dyn Progress>, ) -> VerifyResult { let start = Instant::now(); let mut good: u64 = 0; @@ -97,7 +94,6 @@ pub fn verify_title( let mut bad: u64 = 0; let mut ranges: Vec = Vec::new(); let mut sectors_done: u64 = 0; - let mut _stopped = false; let mut byte_offset: u64 = 0; let total_sectors: u64 = title.extents.iter().map(|e| e.sector_count as u64).sum(); @@ -140,9 +136,21 @@ pub fn verify_title( } sectors_done += count as u64; - if let Some(ref mut cb) = on_progress { - if !cb(sectors_done, total_sectors, status) { - _stopped = true; + if let Some(cb) = on_progress { + let pp = crate::progress::PassProgress { + kind: crate::progress::PassKind::Verify, + work_done: sectors_done, + work_total: total_sectors, + bytes_good_total: (good + slow + recovered) * 2048, + bytes_unreadable_total: bad * 2048, + bytes_pending_total: 0, + bytes_total_disc: total_sectors * 2048, + disc_duration_secs: Some(title.duration_secs), + bytes_bad_in_main_title: 0, + main_title_duration_secs: Some(title.duration_secs), + main_title_size_bytes: Some(total_sectors * 2048), + }; + if !cb.report(&pp) { break 'outer; } } @@ -214,9 +222,21 @@ pub fn verify_title( } sectors_done += 1; - if let Some(ref mut cb) = on_progress { - if !cb(sectors_done, total_sectors, status) { - _stopped = true; + if let Some(cb) = on_progress { + let pp = crate::progress::PassProgress { + kind: crate::progress::PassKind::Verify, + work_done: sectors_done, + work_total: total_sectors, + bytes_good_total: (good + slow + recovered) * 2048, + bytes_unreadable_total: bad * 2048, + bytes_pending_total: 0, + bytes_total_disc: total_sectors * 2048, + disc_duration_secs: Some(title.duration_secs), + bytes_bad_in_main_title: 0, + main_title_duration_secs: Some(title.duration_secs), + main_title_size_bytes: Some(total_sectors * 2048), + }; + if !cb.report(&pp) { break 'outer; } } diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index 538e297..33bd50e 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -181,9 +181,10 @@ fn test_disc_copy_progress_callback_fires() { last_bytes: Arc, } impl libfreemkv::progress::Progress for CountingReporter { - fn report(&self, p: &libfreemkv::progress::PassProgress) { + fn report(&self, p: &libfreemkv::progress::PassProgress) -> bool { self.calls.fetch_add(1, Ordering::Relaxed); self.last_bytes.store(p.bytes_good_total, Ordering::Relaxed); + true } } let reporter = CountingReporter { @@ -680,7 +681,7 @@ fn test_pass_progress_separates_unreadable_from_pending() { dur: Arc, } impl libfreemkv::progress::Progress for SnapshotReporter { - fn report(&self, p: &libfreemkv::progress::PassProgress) { + fn report(&self, p: &libfreemkv::progress::PassProgress) -> bool { self.unreadable .store(p.bytes_unreadable_total, Ordering::Relaxed); self.pending.store(p.bytes_pending_total, Ordering::Relaxed); @@ -688,6 +689,7 @@ fn test_pass_progress_separates_unreadable_from_pending() { if let Some(d) = p.disc_duration_secs { self.dur.store((d * 1000.0) as u64, Ordering::Relaxed); } + true } } let reporter = SnapshotReporter {