From c73a3dbcb62c382dd7142b35c476b14b764fd633 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Mon, 22 Jun 2026 15:02:24 -0700 Subject: [PATCH] Fix Windows multi-drive selection, disk:// alias, and READ chunking Three fixes for a Windows ASUS Blu-ray drive that failed/spammed errors: - resolve.rs: accept disk:// as an alias for disc:// (identical behavior; empty = auto-detect, path = device). Windows users commonly type disk://i: after the drive-letter convention. - drive::find_drive: prefer a drive that reports media present. Enumerate all optical drives, query Drive::drive_status() (GET EVENT STATUS, works regardless of firmware), and return the first reporting DiscPresent; fall back to the first enumerated drive when none report a disc so single-drive / quirky setups don't regress. Selection policy split into select_drive_with_media() for unit testing. - READ chunking: add ScsiTransport::max_transfer_bytes() (default 1 MiB). Windows SPTI overrides it with the adapter MaximumTransferLength queried via IOCTL_STORAGE_QUERY_PROPERTY / StorageAdapterProperty, clamped to a 64 KiB floor (fallback on query failure). Drive::read now caps each READ(10) to that limit: small reads take the unchanged single-CDB path, larger reads loop over read_one() chunks, reporting the failing chunk's LBA on error. This stops the 16 MiB single read that exceeded the adapter limit, made DeviceIoControl fail, and spammed transport-failure warnings with slow tiny-read fallbacks. Tests added for the disk:// alias, media-preference selection, and READ chunk decomposition / per-chunk error LBA. --- src/drive/mod.rs | 260 +++++++++++++++++++++++++++++++++++++++++++- src/mux/resolve.rs | 34 +++++- src/scsi/mod.rs | 17 +++ src/scsi/windows.rs | 106 +++++++++++++++++- 4 files changed, 411 insertions(+), 6 deletions(-) diff --git a/src/drive/mod.rs b/src/drive/mod.rs index 96a6be7..b788388 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -630,6 +630,56 @@ impl Drive { timeout_ms, "Drive::read enter" ); + + // Cap each CDB to the transport's max data-in transfer. A single + // READ larger than the adapter limit fails outright on some + // backends (notably Windows SPTI, where a 16 MiB read exceeds the + // adapter MaximumTransferLength → DeviceIoControl fails → we'd + // mis-read it as a transport failure and spam tiny-read fallbacks). + // For the common small read (count <= max_sectors) this is a single + // read_one call with no behavior change. + let max_sectors = (self.scsi.max_transfer_bytes() / 2048).max(1) as u32; + if count as u32 <= max_sectors { + return self.read_one(lba, count, buf, timeout_ms, recovery); + } + + // Large read: split into chunks of at most `max_sectors` sectors, + // each a self-contained READ(10) with the same validation. Any + // chunk error reports that chunk's LBA (more precise than the whole + // request's base LBA). + let mut done: u32 = 0; + let mut total: usize = 0; + let count = count as u32; + while done < count { + let chunk = (count - done).min(max_sectors); + let cur_lba = lba + done; + let byte_off = done as usize * 2048; + let byte_len = chunk as usize * 2048; + let slice = &mut buf[byte_off..byte_off + byte_len]; + let n = self.read_one(cur_lba, chunk as u16, slice, timeout_ms, recovery)?; + total += n; + done += chunk; + } + Ok(total) + } + + /// Issue a single READ(10) for up to `count` sectors at `lba` into + /// `buf`, with the recovery-timeout already resolved by the caller. + /// This is the byte-identical single-shot read body that `read` calls + /// (once for small reads, in a loop for reads larger than the + /// transport's max transfer). On failure returns `Err(DiscRead)` with + /// `sector = lba` (the failing chunk's LBA) and the preserved SCSI + /// status/sense; a short transfer is treated as a failed read. + fn read_one( + &mut self, + lba: u32, + count: u16, + buf: &mut [u8], + timeout_ms: u32, + // `recovery` only gates the Linux /dev/sr0 pread fallback below; on + // other platforms it is intentionally unused. + #[cfg_attr(not(target_os = "linux"), allow(unused_variables))] recovery: bool, + ) -> Result { let cdb = [ crate::scsi::SCSI_READ_10, 0x00, @@ -900,15 +950,50 @@ impl SectorSource for Drive { } } -/// Find the first optical drive on this system and open it. +/// Find an optical drive on this system and open it, **preferring a drive +/// that currently has media**. +/// +/// On a multi-drive system (common on Windows, where an empty/not-ready +/// drive can enumerate first) returning the first drive blindly can pick a +/// drive with no disc, dooming the operation. So this opens each candidate +/// in enumeration order, queries [`Drive::drive_status`] (GET EVENT STATUS, +/// which works regardless of firmware state), and returns the first drive +/// reporting [`DriveStatus::DiscPresent`]. +/// +/// If no drive reports a disc — or `drive_status()` is unavailable/returns +/// `Unknown` everywhere (single-drive or quirky bridges) — it falls back to +/// the first drive that opened, preserving the historical behavior so those +/// setups don't regress. /// /// For just listing drives without opening (e.g. UI sidebar), use /// `scsi::list_drives()` — that returns `DriveInfo` (path + identity) /// without the cost of running every drive's profile + identity probe. pub fn find_drive() -> Option { - discover_drives() - .into_iter() - .find_map(|(path, _)| Drive::open(std::path::Path::new(&path)).ok()) + select_drive_with_media( + discover_drives() + .into_iter() + .filter_map(|(path, _)| Drive::open(std::path::Path::new(&path)).ok()), + ) +} + +/// Pick a drive from an iterator of opened drives, preferring one whose +/// [`Drive::drive_status`] reports [`DriveStatus::DiscPresent`]. Falls back +/// to the first drive yielded if none report a disc. Split out from +/// [`find_drive`] so the selection policy is unit-testable against fake +/// drives without touching real hardware. +fn select_drive_with_media(drives: impl Iterator) -> Option { + let mut fallback: Option = None; + for mut drive in drives { + if drive.drive_status() == DriveStatus::DiscPresent { + return Some(drive); + } + // Remember the first drive that opened as the no-media fallback so + // single-drive / status-unavailable setups still get a drive. + if fallback.is_none() { + fallback = Some(drive); + } + } + fallback } /// Decode a READ CAPACITY (10) response into a sector count. @@ -1347,6 +1432,173 @@ mod command_tests { assert_eq!(d.read(0, 32, &mut buf, false).unwrap(), 65536); } + // ── Drive::read chunking against a capped transport ───────────── + + /// Transport with a small `max_transfer_bytes` that records the LBA + + /// transfer-length of every READ(10) CDB it sees, reports a full + /// transfer for each, and can be told to fail the Nth read with a SCSI + /// error. Lets a test assert the chunk decomposition and per-chunk + /// error LBA. + struct ChunkingTransport { + max_bytes: usize, + /// Recorded (lba, transfer_length_sectors) per READ(10). + reads: Arc>>, + /// If Some(i), the i-th READ(10) (0-based) fails with a SCSI error. + fail_on: Option, + seen: usize, + } + impl ScsiTransport for ChunkingTransport { + fn max_transfer_bytes(&self) -> usize { + self.max_bytes + } + fn execute( + &mut self, + cdb: &[u8], + _dir: DataDirection, + data: &mut [u8], + _timeout_ms: u32, + ) -> Result { + // Only track READ(10); ignore other CDBs (e.g. the 6-byte + // PREVENT ALLOW MEDIUM REMOVAL the Drive sends on Drop). + if cdb.first() != Some(&crate::scsi::SCSI_READ_10) || cdb.len() < 10 { + return Ok(ScsiResult { + status: 0, + bytes_transferred: data.len(), + sense: [0u8; 32], + }); + } + let lba = u32::from_be_bytes([cdb[2], cdb[3], cdb[4], cdb[5]]); + let count = u16::from_be_bytes([cdb[7], cdb[8]]); + self.reads.lock().unwrap().push((lba, count)); + let idx = self.seen; + self.seen += 1; + if self.fail_on == Some(idx) { + return Err(Error::ScsiError { + opcode: cdb[0], + status: 0x02, + sense: Some(crate::scsi::ScsiSense { + sense_key: 3, + asc: 0x11, + ascq: 0x05, + }), + }); + } + Ok(ScsiResult { + status: 0, + bytes_transferred: data.len(), + sense: [0u8; 32], + }) + } + } + + fn chunking(max_bytes: usize, fail_on: Option) -> (Drive, Arc>>) { + let reads = Arc::new(Mutex::new(Vec::new())); + let t = ChunkingTransport { + max_bytes, + reads: reads.clone(), + fail_on, + seen: 0, + }; + (Drive::from_transport_for_test(Box::new(t)), reads) + } + + #[test] + fn read_chunks_large_request_to_max_transfer() { + // max_transfer = 4 sectors (4 * 2048 = 8192 bytes). A read of 10 + // sectors at LBA 0 must split into 3 READ(10) CDBs: (0,4), (4,4), + // (8,2). The assembled buffer is the full 10*2048 bytes. + let (mut d, reads) = chunking(4 * 2048, None); + let mut buf = vec![0u8; 10 * 2048]; + let n = d.read(0, 10, &mut buf, false).unwrap(); + assert_eq!(n, 10 * 2048, "returns total bytes across all chunks"); + let r = reads.lock().unwrap(); + assert_eq!( + *r, + vec![(0, 4), (4, 4), (8, 2)], + "must chunk into 4+4+2 sectors at advancing LBAs" + ); + } + + #[test] + fn read_chunk_failure_reports_failing_chunk_lba() { + // Same 4-sector cap; fail the 2nd chunk (index 1), which covers + // LBA 4. The error must be DiscRead with sector = 4 (the failing + // chunk's LBA), NOT the request base LBA 0. + let (mut d, reads) = chunking(4 * 2048, Some(1)); + let mut buf = vec![0u8; 10 * 2048]; + let err = d.read(0, 10, &mut buf, false).unwrap_err(); + match err { + Error::DiscRead { sector, status, .. } => { + assert_eq!(sector, 4, "failing chunk's LBA, not the request base"); + assert_eq!(status, Some(0x02)); + } + other => panic!("expected DiscRead, got {other:?}"), + } + // Reads 0 (LBA 0) succeeded and 1 (LBA 4) failed; the loop stops on + // the error so LBA 8 is never issued. + let r = reads.lock().unwrap(); + assert_eq!(*r, vec![(0, 4), (4, 4)], "stops at the failing chunk"); + } + + #[test] + fn read_small_request_is_single_unchunked_read() { + // count <= max_sectors must take the single-read path unchanged: a + // 3-sector read under a 4-sector cap is exactly one READ(10). + let (mut d, reads) = chunking(4 * 2048, None); + let mut buf = vec![0u8; 3 * 2048]; + assert_eq!(d.read(0, 3, &mut buf, false).unwrap(), 3 * 2048); + assert_eq!(*reads.lock().unwrap(), vec![(0, 3)], "single CDB, no split"); + } + + // ── find_drive media-preference selection policy ──────────────── + + /// Build a fake drive whose GET EVENT STATUS reply reports the given + /// media_status byte (byte 5 of an 8-byte reply): 0x02 = DiscPresent, + /// 0x00 = NoDisc, etc. Stands in for a real opened drive so the + /// selection policy is testable without hardware. + fn drive_with_media_byte(media_status: u8) -> Drive { + let mut buf = vec![0u8; 8]; + buf[5] = media_status; + drive_with(buf) + } + + #[test] + fn select_drive_prefers_drive_with_media() { + // Drive #1 has no disc (0x00), drive #2 has a disc (0x02). The + // selection must skip the empty first drive and pick the one with + // media — the Windows multi-drive bug fix. + let drives = vec![drive_with_media_byte(0x00), drive_with_media_byte(0x02)]; + let picked = select_drive_with_media(drives.into_iter()).expect("a drive"); + let mut picked = picked; + assert_eq!( + picked.drive_status(), + DriveStatus::DiscPresent, + "must pick the drive reporting DiscPresent, not the empty first drive" + ); + } + + #[test] + fn select_drive_falls_back_to_first_when_none_have_media() { + // No drive reports a disc → fall back to the FIRST opened drive so + // single-drive / quirky setups still get a drive (historical + // behavior preserved). Tag drive #1 distinctly (TrayOpen 0x01) and + // confirm it, not #2 (NoDisc 0x00), is returned. + let drives = vec![drive_with_media_byte(0x01), drive_with_media_byte(0x00)]; + let mut picked = select_drive_with_media(drives.into_iter()).expect("a fallback drive"); + assert_eq!( + picked.drive_status(), + DriveStatus::TrayOpen, + "fallback must be the first drive yielded" + ); + } + + #[test] + fn select_drive_none_when_no_drives() { + // No candidates at all → None. + let empty: Vec = Vec::new(); + assert!(select_drive_with_media(empty.into_iter()).is_none()); + } + // ── drive_status branch coverage (GET EVENT STATUS byte 5) ────── #[test] diff --git a/src/mux/resolve.rs b/src/mux/resolve.rs index cead84f..579f4f2 100644 --- a/src/mux/resolve.rs +++ b/src/mux/resolve.rs @@ -5,6 +5,7 @@ //! | Scheme | Input | Output | Path | //! |--------|-------|--------|------| //! | disc:// | Yes | -- | empty (auto-detect) or /dev/sgN | +//! | disk:// | Yes | -- | alias for `disc://` (identical behavior) | //! | iso:// | Yes | -- | file path (required) | //! | mkv:// | Yes | Yes | file path (required) | //! | m2ts:// | Yes | Yes | file path (required) | @@ -91,7 +92,13 @@ impl StreamUrl { /// Parse a URL string into a typed StreamUrl. pub fn parse_url(url: &str) -> StreamUrl { - if let Some(rest) = url.strip_prefix("disc://") { + // `disk://` is an accepted alias for `disc://` (identical behavior): + // empty = auto-detect, path = device. Windows users commonly type + // `disk://i:` after the drive-letter convention; honor both spellings. + if let Some(rest) = url + .strip_prefix("disc://") + .or_else(|| url.strip_prefix("disk://")) + { return if rest.is_empty() { StreamUrl::Disc { device: None } } else { @@ -637,14 +644,39 @@ fn build_m2ts_pipeline( #[cfg(test)] mod tests { + use super::StreamUrl; use super::aacs_key_missing; use super::css_key_missing; + use super::parse_url; use super::validate_network_addr; use super::{build_demux_state, build_iso_pipeline, input, output}; use crate::decrypt::DecryptKeys; use crate::disc::{ContentFormat, DiscTitle, Extent}; use crate::pes::Stream as _; use crate::sector::SectorSource; + use std::path::PathBuf; + + #[test] + fn disk_scheme_is_alias_for_disc() { + // `disk://` must parse identically to `disc://`: empty = auto-detect + // (device None), a trailing path = explicit device. A Windows user + // typing `disk://i:` must reach the same live-disc path as `disc://`. + match (parse_url("disk://"), parse_url("disc://")) { + (StreamUrl::Disc { device: a }, StreamUrl::Disc { device: b }) => { + assert_eq!(a, None); + assert_eq!(b, None); + } + other => panic!("disk:// / disc:// must both be Disc, got {other:?}"), + } + match (parse_url("disk://i:"), parse_url("disc://i:")) { + (StreamUrl::Disc { device: a }, StreamUrl::Disc { device: b }) => { + assert_eq!(a, Some(PathBuf::from("i:"))); + assert_eq!(b, Some(PathBuf::from("i:"))); + assert_eq!(a, b, "disk:// device must match disc:// device"); + } + other => panic!("disk://i: / disc://i: must both be Disc, got {other:?}"), + } + } #[test] fn validate_network_addr_rejects_portless() { diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index 1994666..8fc32e6 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -319,6 +319,23 @@ pub trait ScsiTransport: Send { data: &mut [u8], timeout_ms: u32, ) -> Result; + + /// Maximum number of bytes the transport can carry in a single SCSI + /// data-in transfer. A READ that requests more than this must be split + /// into chunks by the caller ([`crate::Drive::read`]) — otherwise the + /// transport fails the whole command. + /// + /// The default is a conservative 1 MiB, safe on every platform. The + /// Windows backend overrides this with the adapter's real + /// `MaximumTransferLength` (queried via `IOCTL_STORAGE_QUERY_PROPERTY`): + /// a 16 MiB READ that exceeds the adapter limit makes + /// `DeviceIoControl` fail outright, which freemkv then mis-reads as a + /// transport failure and falls back to slow, log-spamming tiny reads. + /// Chunking to this limit fixes that. Linux/macOS keep the 1 MiB + /// default (well within any real `max_sectors_kb`). + fn max_transfer_bytes(&self) -> usize { + 1 << 20 + } } // ── Platform-agnostic open / reset ────────────────────────────────────────── diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 6777a11..9eef5e3 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -12,6 +12,19 @@ use std::path::Path; // ── Windows constants ────────────────────────────────────────────────────── const IOCTL_SCSI_PASS_THROUGH_DIRECT: u32 = 0x4D014; +/// IOCTL_STORAGE_QUERY_PROPERTY — CTL_CODE(IOCTL_STORAGE_BASE(0x2D), +/// 0x500, METHOD_BUFFERED(0), FILE_ANY_ACCESS(0)) = 0x002D1400. +const IOCTL_STORAGE_QUERY_PROPERTY: u32 = 0x002D1400; +/// STORAGE_PROPERTY_ID::StorageAdapterProperty. +const STORAGE_ADAPTER_PROPERTY: u32 = 1; +/// STORAGE_QUERY_TYPE::PropertyStandardQuery. +const PROPERTY_STANDARD_QUERY: u32 = 0; + +/// Conservative fallback when the adapter MaximumTransferLength query +/// fails — 64 KiB is universally safe for SPTD on any Windows storage +/// stack. Also the floor we clamp a reported value up to. +const WINDOWS_MIN_TRANSFER_BYTES: usize = 64 * 1024; + const SCSI_IOCTL_DATA_OUT: u8 = 0; const SCSI_IOCTL_DATA_IN: u8 = 1; const SCSI_IOCTL_DATA_UNSPECIFIED: u8 = 2; @@ -54,6 +67,43 @@ struct SptwbDirect { sense: [u8; K_SENSE_SIZE], } +// ── STORAGE_QUERY_PROPERTY structures (winioctl.h) ───────────────────────── + +/// Input to IOCTL_STORAGE_QUERY_PROPERTY. Mirrors `STORAGE_PROPERTY_QUERY`: +/// `{ PropertyId: u32, QueryType: u32, AdditionalParameters: [u8; 1] }`. +#[repr(C)] +#[allow(non_snake_case)] +struct StoragePropertyQuery { + PropertyId: u32, + QueryType: u32, + AdditionalParameters: [u8; 1], +} + +/// Subset of `STORAGE_ADAPTER_DESCRIPTOR` (winioctl.h) up to and including +/// `MaximumTransferLength`. The real struct has more trailing fields, but +/// the driver fills the whole thing and we only read this prefix; reading a +/// truncated descriptor is the documented usage. Field layout (all the +/// leading fields are present so the offset of `MaximumTransferLength` is +/// correct): +/// Version, Size, MaximumTransferLength, MaximumPhysicalPages, +/// AlignmentMask: u32 … +#[repr(C)] +#[allow(non_snake_case)] +struct StorageAdapterDescriptor { + Version: u32, + Size: u32, + MaximumTransferLength: u32, + MaximumPhysicalPages: u32, + AlignmentMask: u32, + AdapterUsesPio: u8, + AdapterScansDown: u8, + CommandQueueing: u8, + AcceleratedTransfer: u8, + BusType: u8, + BusMajorVersion: u16, + BusMinorVersion: u16, +} + // ── Windows FFI ──────────────────────────────────────────────────────────── unsafe extern "system" { @@ -85,6 +135,11 @@ unsafe extern "system" { pub struct SptiTransport { handle: isize, + /// Adapter MaximumTransferLength in bytes, queried once at open via + /// IOCTL_STORAGE_QUERY_PROPERTY and clamped to at least + /// [`WINDOWS_MIN_TRANSFER_BYTES`]. A single READ larger than this fails + /// `DeviceIoControl` outright, so [`crate::Drive::read`] chunks to it. + max_transfer: usize, } // SptiTransport's only field is an isize HANDLE, so the compiler @@ -147,7 +202,12 @@ impl SptiTransport { }); } - Ok(SptiTransport { handle }) + let max_transfer = query_max_transfer_bytes(handle); + + Ok(SptiTransport { + handle, + max_transfer, + }) } /// Reset the drive to a known good state. @@ -243,7 +303,51 @@ impl Drop for SptiTransport { } } +/// Query the storage adapter's `MaximumTransferLength` (bytes) via +/// IOCTL_STORAGE_QUERY_PROPERTY / StorageAdapterProperty. On any failure +/// (IOCTL failed, short reply, or a nonsensical zero) returns the +/// conservative [`WINDOWS_MIN_TRANSFER_BYTES`]; otherwise clamps the +/// reported value up to that floor. Never returns 0. +fn query_max_transfer_bytes(handle: isize) -> usize { + if handle == INVALID_HANDLE_VALUE { + return WINDOWS_MIN_TRANSFER_BYTES; + } + let query = StoragePropertyQuery { + PropertyId: STORAGE_ADAPTER_PROPERTY, + QueryType: PROPERTY_STANDARD_QUERY, + AdditionalParameters: [0u8; 1], + }; + let mut desc: StorageAdapterDescriptor = unsafe { std::mem::zeroed() }; + let mut bytes_returned: u32 = 0; + let ok = unsafe { + DeviceIoControl( + handle, + IOCTL_STORAGE_QUERY_PROPERTY, + &query as *const _ as *mut std::ffi::c_void, + std::mem::size_of::() as u32, + &mut desc as *mut _ as *mut std::ffi::c_void, + std::mem::size_of::() as u32, + &mut bytes_returned, + std::ptr::null_mut(), + ) + }; + // MaximumTransferLength sits at offset 8; need at least that many bytes + // written for the field to be valid. + let valid = ok != 0 + && bytes_returned as usize + >= std::mem::offset_of!(StorageAdapterDescriptor, MaximumTransferLength) + + std::mem::size_of::(); + if !valid || desc.MaximumTransferLength == 0 { + return WINDOWS_MIN_TRANSFER_BYTES; + } + (desc.MaximumTransferLength as usize).max(WINDOWS_MIN_TRANSFER_BYTES) +} + impl ScsiTransport for SptiTransport { + fn max_transfer_bytes(&self) -> usize { + self.max_transfer + } + fn execute( &mut self, cdb: &[u8],