diff --git a/CHANGELOG.md b/CHANGELOG.md index c86c538..b799f32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,81 @@ # Changelog +## 0.13.23 (2026-04-27) + +### Stop discarding the drive's SCSI sense data + +Through the entire 0.13.x line, every CHECK CONDITION reply from the +drive (the standard way SCSI reports a sector failure) was being +collapsed into a synthetic `status=0xFF, sense_key=0` "transport +wedge" sentinel and the real sense data was thrown away. Live tracing +on the BU40N reading Dune 2 on 2026-04-27 confirmed it: the drive was +returning `host_status=0, driver_status=8, status=2, exec_elapsed_ms=1416` +on every bad sector — a clean CHECK CONDITION carrying full sense +data — and the library was misclassifying it as a wedge and bailing. + +Root cause: `scsi/linux.rs`'s wedge check was `host_status != 0 || +driver_status != 0`. SG's `DRIVER_SENSE` bit (0x08) is set on every +CHECK CONDITION reply just to flag "sense buffer is populated" — it's +not a transport failure on its own. Pre-0.13.23 we conflated the two +and silently lost every drive-reported error reason. macOS and Windows +backends had the same shape: they extracted `sense_key` only, dropping +ASC/ASCQ. + +### What 0.13.23 changes (API) + +- **Linux**: mask `DRIVER_SENSE` before treating `driver_status` as a + transport-layer failure. Real transport failures (`host_status != 0` + or any non-SENSE bit set) still synthesise the `0xFF` sentinel. +- **`Error::ScsiError`** carries `sense: Option` instead of + flat `sense_key`/`asc`/`ascq`. `sense=None` ⇔ transport failure (no + SCSI status delivered). `Some(ScsiSense {…})` ⇔ drive replied with + sense data. Removes the `0xFF`/`sense_key=0` magic-number coupling. +- **`ScsiSense`** is a public type with predicate methods on it — + `is_marginal`, `is_medium_error`, `is_hardware_error`, + `is_unit_attention`, `is_data_protect`, `is_not_ready`, + `is_illegal_request`, `is_aborted_command`. Callers route on the + structured fields rather than raw key comparisons. +- **`Error::scsi_sense()`** / **`Error::is_scsi_transport_failure()`** / + **`Error::is_marginal_read()`** convenience predicates on `Error`. + `is_marginal_read` is the high-level "should `Disc::copy` engage + hysteresis on this error?" check. +- **SCSI protocol constants** (`SCSI_STATUS_GOOD`, + `SCSI_STATUS_CHECK_CONDITION`, `SCSI_STATUS_TRANSPORT_FAILURE`, + `SENSE_KEY_*`) moved from `error.rs` to `scsi/mod.rs` where they + belong alongside `SCSI_INQUIRY`, `SCSI_READ_10`, etc. +- **macOS** + **Windows** backends parse the full sense triple too. + Same code path on every platform — a regression in `parse_sense` + would surface on all three OSes simultaneously. +- **`parse_sense`** replaces `parse_sense_key` (returns the full + triple). Inline sense-format tests (descriptor 0x72/0x73 vs fixed + 0x70/0x71, short-buffer, VALID-bit masking, unknown response codes) + now also exercise ASC/ASCQ extraction at the right offsets. + +### Disc::copy + Disc::patch sense-aware dispatch + +Both passes now bail immediately when a read fails with a sense class +that retry can't help (HARDWARE ERROR, DATA PROTECT, UNIT ATTENTION, +NOT READY, ILLEGAL REQUEST, real transport failure, kernel `IoError`) +rather than burning hysteresis cycles on a doomed loop. Marginal-read +sense (MEDIUM ERROR, ABORTED COMMAND, RECOVERED ERROR, NO SENSE) +engages hysteresis as before. New `phase=bail` trace event records +the bail reason. + +`Disc::patch`'s `wedged_threshold` (50 consecutive failures) remains +as defense-in-depth for chains of marginal failures, but a single +non-marginal sense now short-circuits it. + +### Behavioural impact + +For damaged-disc rips on the BU40N this unblocks v0.13.22's +hysteresis: pre-fix, the misclassified "wedge" caused `Disc::copy` to +exit before hysteresis could engage, so `bytes_good` froze at the bad +zone. Post-fix the drive's CHECK CONDITION replies flow through the +normal path → hysteresis drops to bpt=1 → marginal sectors are +recovered or marked Unreadable. Calibration data +(`docs/audits/2026-04-26-bisect-on-fail-empirical-findings.md`) shows +~86 % of marginal-region sectors recover at bpt=1 on this drive. + ## 0.13.22 (2026-04-26) ### Replace bisect-on-fail with hysteresis state machine (Block ↔ Single) diff --git a/Cargo.toml b/Cargo.toml index fceb310..142e6d3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "libfreemkv" -version = "0.13.22" +version = "0.13.23" edition = "2024" rust-version = "1.86" license = "AGPL-3.0-only" diff --git a/src/disc/mod.rs b/src/disc/mod.rs index e175076..cd53005 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -1382,16 +1382,14 @@ impl Disc { let read_t0 = block_t0; let block_bytes_usz = block_bytes as usize; iter_count += 1; - let block_ok = reader - .read_sectors( - block_lba, - block_count, - &mut buf[..block_bytes_usz], - recovery, - ) - .is_ok(); + let block_result = reader.read_sectors( + block_lba, + block_count, + &mut buf[..block_bytes_usz], + recovery, + ); - if block_ok { + if block_result.is_ok() { // Fast path — full block read cleanly. read_ok_count += 1; if opts.decrypt { @@ -1413,6 +1411,31 @@ impl Disc { return Err(Error::DiscRead { sector: block_lba as u64, }); + } else if !block_result + .as_ref() + .err() + .map(Error::is_marginal_read) + .unwrap_or(false) + { + // 0.13.23: SCSI sense-aware dispatch. Block read failed + // with a sense class outside the marginal-read set + // (real transport failure, HARDWARE ERROR, DATA + // PROTECT, UNIT ATTENTION, NOT READY, ILLEGAL + // REQUEST, kernel IoError). Bail with the full sense + // triple preserved — caller (autorip) surfaces + // "physical replug needed" / "drive failing" / + // "media changed" / etc to the user. Hysteresis + // would just hammer the same failure for thousands + // of sectors at 1.4 sec each. + let err = block_result.err().unwrap(); + tracing::trace!( + target: "freemkv::disc", + phase = "bail", + lba = block_lba, + error = %err, + "block read failed with non-recoverable sense; bailing" + ); + return Err(err); } else { // Block failed → drop to Single and read this range // sector-by-sector. Stay in Single across subsequent @@ -1445,9 +1468,30 @@ impl Disc { let s_lba = block_lba + s as u32; let s_pos = pos + (s as u64) * 2048; let one_bytes = 2048usize; - let one_ok = reader - .read_sectors(s_lba, 1, &mut buf[..one_bytes], recovery) - .is_ok(); + let one_result = + reader.read_sectors(s_lba, 1, &mut buf[..one_bytes], recovery); + // 0.13.23: same sense-aware dispatch inside Single + // mode. If a single-sector read fails with a + // non-marginal sense (transport / hardware / + // DATA PROTECT / UNIT ATTENTION / NOT READY / + // ILLEGAL REQUEST / kernel IoError), the drive + // isn't going to start succeeding for the next + // 60 sectors either — bail with full sense info + // rather than chewing through bpt=1 timeouts. + if let Err(ref e) = one_result { + if !e.is_marginal_read() { + let err = one_result.err().unwrap(); + tracing::trace!( + target: "freemkv::disc", + phase = "bail", + lba = s_lba, + error = %err, + "bpt=1 read failed with non-marginal sense; bailing" + ); + return Err(err); + } + } + let one_ok = one_result.is_ok(); if one_ok { read_ok_count += 1; consecutive_good = consecutive_good.saturating_add(1); @@ -1769,9 +1813,30 @@ impl Disc { let count = (block_bytes / 2048) as u16; let bytes = count as usize * 2048; blocks_attempted += 1; - let read_ok = reader - .read_sectors(lba, count, &mut buf[..bytes], recovery) - .is_ok(); + let read_result = reader.read_sectors(lba, count, &mut buf[..bytes], recovery); + // 0.13.23: parity with Disc::copy — bail immediately on a + // non-marginal SCSI sense (transport failure, HARDWARE + // ERROR, DATA PROTECT, UNIT ATTENTION, NOT READY, + // ILLEGAL REQUEST, kernel IoError). The wedged_threshold + // counter is a defense-in-depth backstop for ~50 + // consecutive marginal failures, but a single + // non-marginal error already proves the drive can't + // produce data this pass — don't waste 50× the timeout + // budget proving it again. + if let Err(ref e) = read_result { + if !e.is_marginal_read() { + let err = read_result.err().unwrap(); + tracing::trace!( + target: "freemkv::disc", + phase = "patch_bail", + lba, + error = %err, + "patch read failed with non-marginal sense; bailing" + ); + return Err(err); + } + } + let read_ok = read_result.is_ok(); if read_ok { blocks_read_ok += 1; consecutive_failures = 0; diff --git a/src/drive/mod.rs b/src/drive/mod.rs index 3fed6f3..343cb44 100644 --- a/src/drive/mod.rs +++ b/src/drive/mod.rs @@ -241,8 +241,9 @@ impl Drive { 5_000, ) { Ok(_) => DriveStatus::DiscPresent, - Err(Error::ScsiError { sense_key: 2, .. }) => DriveStatus::NotReady, - Err(Error::ScsiError { sense_key: 6, .. }) => DriveStatus::NotReady, // UNIT ATTENTION + Err(ref e) if e.scsi_sense().is_some_and(|s| s.is_not_ready() || s.is_unit_attention()) => { + DriveStatus::NotReady + } _ => DriveStatus::Unknown, } } diff --git a/src/error.rs b/src/error.rs index ff8a44f..da7080d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -155,10 +155,24 @@ pub enum Error { }, // SCSI (4xxx) + /// SCSI command failed. + /// + /// `opcode` is the failing CDB byte 0. `status` is the raw SCSI + /// status byte: `0x02` = CHECK CONDITION (drive replied with sense + /// data), `0xFF` = libfreemkv-synthesised sentinel meaning "no SCSI + /// status delivered" (kernel timeout, USB bridge wedge, IOKit + /// service failure). `sense` carries the drive's SPC-4 sense triple + /// when the drive replied; `None` for transport-layer failures. + /// + /// Recommended dispatch (callers shouldn't pattern-match raw + /// fields): + /// - [`Error::is_scsi_transport_failure`] — bail; bridge/transport wedge + /// - [`Error::is_marginal_read`] — drive said this read was marginal; smaller block may recover + /// - [`Error::scsi_sense`] — borrow the sense triple for finer routing ([`ScsiSense::is_medium_error`] etc.) ScsiError { opcode: u8, status: u8, - sense_key: u8, + sense: Option, }, // I/O (5xxx) @@ -363,17 +377,26 @@ impl std::fmt::Display for Error { Error::ScsiError { opcode, status, - sense_key, - } => { - write!( + sense, + } => match sense { + Some(s) => write!( f, - "E{}: 0x{:02x}/0x{:02x}/0x{:02x}", + "E{}: 0x{:02x}/0x{:02x}/0x{:02x}/0x{:02x}/0x{:02x}", self.code(), opcode, status, - sense_key - ) - } + s.sense_key, + s.asc, + s.ascq, + ), + None => write!( + f, + "E{}: 0x{:02x}/0x{:02x}", + self.code(), + opcode, + status, + ), + }, Error::IoError { source } => write!(f, "E{}: {}", self.code(), source), Error::DiscRead { sector } => write!(f, "E{}: {}", self.code(), sector), Error::Halted => write!(f, "E{}", self.code()), @@ -438,6 +461,58 @@ impl From for std::io::Error { /// Convenience alias for `Result`. pub type Result = std::result::Result; +impl Error { + /// Borrow the drive-returned SPC-4 sense triple if this error is a + /// [`Error::ScsiError`] carrying sense data. `None` for any other + /// variant **and** for `ScsiError`s that represent a transport-layer + /// failure (where the device never delivered a SCSI status reply, so + /// no sense data exists). + pub fn scsi_sense(&self) -> Option<&crate::scsi::ScsiSense> { + match self { + Error::ScsiError { + sense: Some(s), .. + } => Some(s), + _ => None, + } + } + + /// True if this is a [`Error::ScsiError`] representing a transport-layer + /// failure — kernel timeout, USB bridge wedge, IOKit service error. + /// The device never delivered a SCSI status reply, so there is no + /// sense data to inspect; retrying typically requires physical + /// intervention (replug). + pub fn is_scsi_transport_failure(&self) -> bool { + matches!( + self, + Error::ScsiError { + status: crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE, + .. + } + ) + } + + /// True if the underlying SCSI failure is a *marginal read* — the + /// drive returned an error category in which smaller-granularity + /// retries can sometimes recover the data: + /// + /// - MEDIUM ERROR (sense key 3) — canonical bad-sector signal + /// - ABORTED COMMAND (sense key B) — transient; retry usually works + /// - RECOVERED ERROR (sense key 1) / NO SENSE (sense key 0) — not + /// classified as fatal; treat as recoverable + /// + /// Returns `false` for transport failures (no sense data delivered), + /// HARDWARE ERROR, DATA PROTECT, UNIT ATTENTION, NOT READY, ILLEGAL + /// REQUEST, BLANK CHECK, kernel `IoError`, and any non-SCSI variant. + /// Caller-agnostic predicate — describes a property of the *error*, + /// not what one specific call site should do with it. Used by + /// `Disc::copy`'s hysteresis dispatch. + pub fn is_marginal_read(&self) -> bool { + self.scsi_sense() + .map(crate::scsi::ScsiSense::is_marginal) + .unwrap_or(false) + } +} + #[cfg(test)] mod tests { //! Smoke tests for the error code → variant mapping. Each new variant diff --git a/src/lib.rs b/src/lib.rs index d0687ab..191d3c6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -169,7 +169,7 @@ pub use mux::{InputOptions, StreamUrl, input, output, parse_url}; // out-of-tree platform backends. `SectorReader` lets callers feed any byte // source (test harness, network image, SMB share) into the disc scan // pipeline; `FileSectorReader` is the standard ISO-on-disk implementation. -pub use scsi::{DriveInfo, ScsiTransport, drive_has_disc, list_drives}; +pub use scsi::{DriveInfo, ScsiSense, ScsiTransport, drive_has_disc, list_drives}; pub use sector::{FileSectorReader, SectorReader}; pub use speed::DriveSpeed; pub use udf::{UdfFs, read_filesystem}; diff --git a/src/platform/mt1959/mod.rs b/src/platform/mt1959/mod.rs index 85c0b2d..806e46a 100644 --- a/src/platform/mt1959/mod.rs +++ b/src/platform/mt1959/mod.rs @@ -96,8 +96,8 @@ impl Mt1959 { if result.bytes_transferred != expected { return Err(Error::ScsiError { opcode: SCSI_READ_BUFFER, - status: 0xFF, - sense_key: 0, + status: crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } Ok(result.bytes_transferred) @@ -169,8 +169,8 @@ impl Mt1959 { } Err(Error::ScsiError { opcode: SCSI_READ_BUFFER, - status: 0xFF, - sense_key: 0, + status: crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }) } @@ -275,8 +275,8 @@ impl Mt1959 { { return Err(Error::ScsiError { opcode: SCSI_READ_BUFFER, - status: 0xFF, - sense_key: 0, + status: crate::scsi::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } addr = addr.wrapping_add(PROBE_STEP); diff --git a/src/scsi/linux.rs b/src/scsi/linux.rs index 5d598be..cfe876e 100644 --- a/src/scsi/linux.rs +++ b/src/scsi/linux.rs @@ -243,10 +243,19 @@ impl ScsiTransport for SgIoTransport { /// Errors we surface to caller (any of these = command failed): /// /// - ioctl returned -1 → `Error::IoError` (kernel-level failure) - /// - `hdr.host_status` != 0 → `Error::ScsiError` with status=0xFF - /// (transport-level: timeout, bridge wedge, etc.) - /// - `hdr.driver_status` != 0 → `Error::ScsiError` with status=0xFF - /// - `hdr.status` != 0 → `Error::ScsiError` with parsed sense key + /// - `hdr.host_status` != 0 OR `(hdr.driver_status & ~DRIVER_SENSE)` != 0 + /// → `Error::ScsiError { status: 0xFF, sense_key: 0, asc: 0, ascq: 0 }` + /// (real transport-layer failure: kernel timeout, bridge wedge, bus error) + /// - `hdr.status` != 0 (typically `0x02` CHECK CONDITION) → + /// `Error::ScsiError { status, sense_key, asc, ascq }` carrying the + /// drive's full SPC-4 sense triple. Callers route on + /// `is_medium_error()`, `is_unit_attention()`, etc. + /// + /// Note: SG's `DRIVER_SENSE` (0x08) bit indicates *sense data is + /// attached* — it's set on every CHECK CONDITION reply. It is **not** + /// a transport failure; pre-0.13.23 we conflated it with one and + /// silently lost every drive-reported error reason. The mask in the + /// transport-error check below is the fix. /// /// Caller's `data` buffer is mutated only on success; partial /// transfers are reported via `bytes_transferred = data.len() - resid`. @@ -272,8 +281,8 @@ impl ScsiTransport for SgIoTransport { if data.len() > u32::MAX as usize { return Err(Error::ScsiError { opcode: cdb[0], - status: 0xFF, - sense_key: 0, + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } @@ -323,7 +332,16 @@ impl ScsiTransport for SgIoTransport { // bus error). `hdr.status` may still be zero — the SCSI device // never got to send a status byte. Surface as 0xFF so callers // (e.g. `drive_has_disc`) can detect the wedge signature. - if hdr.host_status != 0 || hdr.driver_status != 0 { + // + // 0.13.23: mask out `DRIVER_SENSE` (0x08) before treating + // `driver_status` as a transport failure. That bit is set on + // *every* CHECK CONDITION reply just to flag "sense data is + // attached in `sbp`" — it's not an error of its own. Pre-fix + // we collapsed every drive-reported error into a synthetic + // 0xFF wedge signature and discarded the sense data, which + // killed the rip's classification logic on damaged discs. + let driver_status_real = hdr.driver_status & !super::DRIVER_SENSE; + if hdr.host_status != 0 || driver_status_real != 0 { tracing::trace!( target: "freemkv::scsi", phase = "transport_err", @@ -336,28 +354,32 @@ impl ScsiTransport for SgIoTransport { ); return Err(Error::ScsiError { opcode: cdb[0], - status: 0xFF, - sense_key: 0, + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } - // SCSI-level failure: device responded, returned non-zero status. - // Parse sense key for the caller. + // SCSI-level failure: device responded, returned non-zero status + // (typically 0x02 CHECK CONDITION). Parse the full SPC-4 sense + // triple so callers can route on `ScsiSense::is_medium_error()` + // etc. if hdr.status != 0 { - let sense_key = super::parse_sense_key(&sense, hdr.sb_len_wr); + let parsed = super::parse_sense(&sense, hdr.sb_len_wr); tracing::trace!( target: "freemkv::scsi", phase = "scsi_err", opcode = opcode, status = hdr.status, - sense_key, + sense_key = parsed.sense_key, + asc = parsed.asc, + ascq = parsed.ascq, exec_elapsed_ms, "SCSI status non-zero" ); return Err(Error::ScsiError { opcode: cdb[0], status: hdr.status, - sense_key, + sense: Some(parsed), }); } @@ -394,11 +416,6 @@ impl ScsiTransport for SgIoTransport { /// Stored in `/sys/class/scsi_generic/sgN/device/type` as ASCII decimal. const SCSI_TYPE_OPTICAL: &str = "5"; -/// SCSI sense key 2 = "NOT READY". Sub-codes distinguish "medium not present" -/// (no disc) from other not-ready states (loading, etc.); for poll-loop -/// purposes any sense-key 2 means "no disc to act on". -const SENSE_KEY_NOT_READY: u8 = 2; - /// Maximum sg index probed in the fallback path when sysfs is unavailable. /// Linux assigns `/dev/sgN` sequentially per host adapter; 16 covers any /// realistic homelab (typical PERC + USB optical = ≤8 nodes). @@ -547,10 +564,7 @@ pub(super) fn drive_has_disc(path: &Path) -> Result { crate::scsi::TUR_TIMEOUT_MS, ) { Ok(_) => Ok(true), - Err(Error::ScsiError { - sense_key: SENSE_KEY_NOT_READY, - .. - }) => Ok(false), + Err(ref e) if e.scsi_sense().is_some_and(|s| s.is_not_ready()) => Ok(false), Err(e) => Err(e), } } diff --git a/src/scsi/macos.rs b/src/scsi/macos.rs index 8eda238..7068431 100644 --- a/src/scsi/macos.rs +++ b/src/scsi/macos.rs @@ -337,13 +337,11 @@ pub(super) fn drive_has_disc(path: &Path) -> Result { crate::scsi::TUR_TIMEOUT_MS, ) { Ok(_) => Ok(true), - Err(Error::ScsiError { sense_key, .. }) if sense_key == K_SENSE_KEY_NOT_READY => Ok(false), + Err(ref e) if e.scsi_sense().is_some_and(|s| s.is_not_ready()) => Ok(false), Err(e) => Err(e), } } -const K_SENSE_KEY_NOT_READY: u8 = 2; - impl Drop for MacScsiTransport { fn drop(&mut self) { if self.device_iface.is_null() { @@ -377,8 +375,8 @@ impl ScsiTransport for MacScsiTransport { if task.is_null() { return Err(Error::ScsiError { opcode: cdb[0], - status: 0xFF, - sense_key: 0, + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } @@ -449,20 +447,25 @@ impl ScsiTransport for MacScsiTransport { // the kernel mid-layer has already done what it can. return Err(Error::ScsiError { opcode: cdb[0], - status: 0xFF, - sense_key: 0, + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } if task_status != K_SCSI_TASK_STATUS_GOOD as u32 { // IOKit doesn't surface a "bytes written into sense buffer" // count the way SG_IO does — pass the buffer's full length - // and let parse_sense_key inspect byte 0's response code. - let sense_key = super::parse_sense_key(&sense, sense.len() as u8); + // and let parse_sense inspect byte 0's response code to pick + // descriptor-vs-fixed format. + // + // 0.13.23: carry the full SPC-4 sense triple in + // `Error::ScsiError::sense` so callers can route on + // `ScsiSense::is_medium_error()` etc. + let parsed = super::parse_sense(&sense, sense.len() as u8); return Err(Error::ScsiError { opcode: cdb[0], status: task_status as u8, - sense_key, + sense: Some(parsed), }); } diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index 764c933..96eb033 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -75,36 +75,207 @@ pub(crate) const READ_TIMEOUT_MS: u32 = 10_000; /// (`DEF_TIMEOUT = 60000`). pub(crate) const READ_RECOVERY_TIMEOUT_MS: u32 = 60_000; -// ── Sense-key parsing ─────────────────────────────────────────────────────── +// ── SCSI status bytes (SPC-4 §4.5.5) ──────────────────────────────────────── -/// Extract the SPC-4 sense key from a sense buffer. +/// Status byte 0x00 — `GOOD`. Command completed successfully. +pub const SCSI_STATUS_GOOD: u8 = 0x00; +/// Status byte 0x02 — `CHECK CONDITION`. Drive completed the command +/// reply and attached sense data describing the failure. +pub const SCSI_STATUS_CHECK_CONDITION: u8 = 0x02; +/// libfreemkv-synthesised sentinel: the transport never delivered a +/// SCSI status byte (kernel timeout, USB bridge wedge, IOKit service +/// failure). Distinct from any drive-returned value. Carriers +/// [`Error::ScsiError`] with `sense = None`. +pub const SCSI_STATUS_TRANSPORT_FAILURE: u8 = 0xFF; + +// ── SPC-4 sense keys (§4.5.6 Table 28) ───────────────────────────────────── +// +// Broad failure category returned in a CHECK CONDITION reply's sense data. +// Names match the SCSI spec; predicate methods on [`ScsiSense`] (e.g. +// `is_medium_error`, `is_unit_attention`) read more fluently than raw +// constant comparisons at call sites. + +pub const SENSE_KEY_NO_SENSE: u8 = 0x00; +pub const SENSE_KEY_RECOVERED_ERROR: u8 = 0x01; +pub const SENSE_KEY_NOT_READY: u8 = 0x02; +pub const SENSE_KEY_MEDIUM_ERROR: u8 = 0x03; +pub const SENSE_KEY_HARDWARE_ERROR: u8 = 0x04; +pub const SENSE_KEY_ILLEGAL_REQUEST: u8 = 0x05; +pub const SENSE_KEY_UNIT_ATTENTION: u8 = 0x06; +pub const SENSE_KEY_DATA_PROTECT: u8 = 0x07; +pub const SENSE_KEY_BLANK_CHECK: u8 = 0x08; +pub const SENSE_KEY_ABORTED_COMMAND: u8 = 0x0B; + +// ── Sense parsing ─────────────────────────────────────────────────────────── + +/// Decoded SPC-4 sense triple — the precise reason a SCSI command failed. /// -/// Handles both response-code formats: -/// - Descriptor format (0x72 / 0x73): sense key in the low nibble of byte 1. -/// - Fixed format (0x70 / 0x71): sense key in the low nibble of byte 2. +/// Returned by [`parse_sense`] and embedded inside [`Error::ScsiError`] +/// (`sense: Option`). Predicate methods (`is_medium_error`, +/// `is_unit_attention`, `is_marginal`, …) read more fluently at call +/// sites than raw `sense_key` comparisons. /// -/// `sb_len_wr` is the number of bytes the transport actually wrote into -/// `sense`. When < 3 (or `sense.len() < 3`) we can't safely read either -/// the format byte or the key byte — return 0 (NO SENSE) per SPC-4 §4.5.3. -/// -/// Pure function. Same parse runs on every platform backend so -/// callers don't have to special-case Linux SG_IO vs macOS IOKit vs -/// Windows SPTI sense layouts. -pub(crate) fn parse_sense_key(sense: &[u8], sb_len_wr: u8) -> u8 { - if (sb_len_wr as usize) < 3 || sense.len() < 3 { - return 0; +/// `Default::default()` and the [`ScsiSense::NONE`] constant both +/// produce the all-zero "no sense info" triple. Per SPC-4 §4.5.3, an +/// empty sense buffer is reported as NO SENSE (key 0); use the constant +/// for explicit intent at construction sites. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct ScsiSense { + /// Sense key — broad failure category (SPC-4 §4.5.6 Table 28). + /// See the `SENSE_KEY_*` constants for named values. + pub sense_key: u8, + /// Additional Sense Code — narrows the cause within a sense key + /// (SPC-4 §4.5.6 Table 29). E.g. `0x11` = UNRECOVERED READ ERROR. + pub asc: u8, + /// Additional Sense Code Qualifier — finest-grain disambiguation. + /// E.g. `0x05` (with `asc=0x11`) = L-EC UNCORRECTABLE. + pub ascq: u8, +} + +impl ScsiSense { + /// Sense reply with all-zero fields — explicit "no sense info" + /// constructor for sites where `Default::default()` would be opaque. + pub const NONE: ScsiSense = ScsiSense { + sense_key: 0, + asc: 0, + ascq: 0, + }; + + /// `true` when the sense key indicates a *marginal-read* failure — + /// the kind of error where the same read at smaller granularity + /// (or a brief retry) sometimes succeeds: + /// + /// - `MEDIUM ERROR` (3) — canonical bad-sector signal + /// - `ABORTED COMMAND` (B) — transient; retry usually works + /// - `RECOVERED ERROR` (1) / `NO SENSE` (0) — drive is healthy and + /// either recovered the data or has no specific fault to report + /// + /// `false` for HARDWARE ERROR, DATA PROTECT, UNIT ATTENTION, NOT + /// READY, ILLEGAL REQUEST, BLANK CHECK, and any unknown key. Used + /// by [`Error::is_marginal_read`] / `Disc::copy`'s hysteresis + /// dispatch. + pub fn is_marginal(&self) -> bool { + matches!( + self.sense_key, + SENSE_KEY_NO_SENSE + | SENSE_KEY_RECOVERED_ERROR + | SENSE_KEY_MEDIUM_ERROR + | SENSE_KEY_ABORTED_COMMAND + ) } - let response_code = sense[0] & 0x7F; - if response_code == 0x72 || response_code == 0x73 { - sense[1] & 0x0F - } else { - // Fixed format (0x70/0x71) and any unknown code fall through here; - // SPC-4 says implementations MUST tolerate unknown response codes - // and treat them as fixed — matches what reference projects do. - sense[2] & 0x0F + + /// `true` if `sense_key == MEDIUM ERROR (3)` — canonical "bad sector" + /// signal from the drive. + pub fn is_medium_error(&self) -> bool { + self.sense_key == SENSE_KEY_MEDIUM_ERROR + } + + /// `true` if `sense_key == HARDWARE ERROR (4)` — drive itself is + /// failing. Not recoverable by retry. + pub fn is_hardware_error(&self) -> bool { + self.sense_key == SENSE_KEY_HARDWARE_ERROR + } + + /// `true` if `sense_key == NOT READY (2)` — medium not present / + /// drive becoming ready / etc. + pub fn is_not_ready(&self) -> bool { + self.sense_key == SENSE_KEY_NOT_READY + } + + /// `true` if `sense_key == UNIT ATTENTION (6)` — disc/drive state + /// changed since the prior command (media inserted/removed, + /// power-on reset, parameters changed). Caller should rescan rather + /// than retry the read. + pub fn is_unit_attention(&self) -> bool { + self.sense_key == SENSE_KEY_UNIT_ATTENTION + } + + /// `true` if `sense_key == DATA PROTECT (7)` — read blocked by + /// AACS / region / write-protect. Retry won't help. + pub fn is_data_protect(&self) -> bool { + self.sense_key == SENSE_KEY_DATA_PROTECT + } + + /// `true` if `sense_key == ILLEGAL REQUEST (5)` — typically a bug + /// in the CDB we sent (LBA out of range, reserved bit, etc.). Don't + /// retry. + pub fn is_illegal_request(&self) -> bool { + self.sense_key == SENSE_KEY_ILLEGAL_REQUEST + } + + /// `true` if `sense_key == ABORTED COMMAND (B)` — transient; one + /// retry is usually safe. + pub fn is_aborted_command(&self) -> bool { + self.sense_key == SENSE_KEY_ABORTED_COMMAND } } +/// Decode an SPC-4 sense buffer into the structured triple +/// `(sense_key, asc, ascq)`. +/// +/// Handles both response-code formats SPC-4 mandates: +/// +/// - **Descriptor format** (response code `0x72` / `0x73`): +/// - sense key = `sense[1] & 0x0F` +/// - asc = `sense[2]` +/// - ascq = `sense[3]` +/// - **Fixed format** (response code `0x70` / `0x71` and any unknown +/// code per SPC-4 §4.5.3): +/// - sense key = `sense[2] & 0x0F` +/// - asc = `sense[12]` +/// - ascq = `sense[13]` +/// +/// `sb_len_wr` is the number of bytes the transport actually wrote into +/// `sense`. When the buffer is too short for the relevant fields we +/// return [`ScsiSense::NONE`] for the missing pieces rather than reading +/// uninitialised memory. The minimum useful sense reply per SPC-4 is 8 +/// bytes (descriptor) or 14 bytes (fixed, to reach ASC/ASCQ at offsets +/// 12/13). +/// +/// Pure function — same parse on every platform backend (Linux SG_IO, +/// macOS IOKit, Windows SPTI) so a regression here would silently +/// mis-route SCSI errors on all three OSes simultaneously. +pub(crate) fn parse_sense(sense: &[u8], sb_len_wr: u8) -> ScsiSense { + let n = (sb_len_wr as usize).min(sense.len()); + if n < 3 { + return ScsiSense::NONE; + } + let response_code = sense[0] & 0x7F; + let descriptor = response_code == 0x72 || response_code == 0x73; + if descriptor { + // Descriptor format: key/asc/ascq are at fixed offsets 1/2/3. + let asc = if n >= 3 { sense[2] } else { 0 }; + let ascq = if n >= 4 { sense[3] } else { 0 }; + ScsiSense { + sense_key: sense[1] & 0x0F, + asc, + ascq, + } + } else { + // Fixed format: key at byte 2, ASC/ASCQ at bytes 12/13. + let asc = if n >= 13 { sense[12] } else { 0 }; + let ascq = if n >= 14 { sense[13] } else { 0 }; + ScsiSense { + sense_key: sense[2] & 0x0F, + asc, + ascq, + } + } +} + +// ── SG_IO driver_status bits ──────────────────────────────────────────────── + +/// `DRIVER_SENSE` (0x08) — bit set in `driver_status` to indicate that +/// sense data was attached to a CHECK CONDITION reply. **Not** a transport +/// failure on its own. Mask this off before deciding whether `driver_status` +/// represents a real bus/host problem. +/// +/// Used by Linux SG_IO (`sg_io_hdr.driver_status`); macOS IOKit and +/// Windows SPTI carry the equivalent signal in different fields and +/// don't need the same masking — the misclassification was Linux-only. +#[cfg(target_os = "linux")] +pub(crate) const DRIVER_SENSE: u16 = 0x08; + // ── Types ─────────────────────────────────────────────────────────────────── #[derive(Debug, Clone, Copy, PartialEq)] @@ -396,7 +567,10 @@ mod parse_sense_tests { //! same helper runs on every platform backend so a regression here //! would silently miscategorize SCSI errors on Linux, macOS, and //! Windows simultaneously. - use super::parse_sense_key; + use super::parse_sense; + fn parse_sense_key(sense: &[u8], sb_len_wr: u8) -> u8 { + parse_sense(sense, sb_len_wr).sense_key + } /// Helper: build a 32-byte sense buffer whose first three bytes are /// the given prefix; the rest are zeroes (sense data area). diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs index 2ed5d4f..a7be236 100644 --- a/src/scsi/windows.rs +++ b/src/scsi/windows.rs @@ -228,7 +228,7 @@ pub(super) fn drive_has_disc(path: &Path) -> Result { crate::scsi::TUR_TIMEOUT_MS, ) { Ok(_) => Ok(true), - Err(Error::ScsiError { sense_key: 2, .. }) => Ok(false), + Err(ref e) if e.scsi_sense().is_some_and(|s| s.is_not_ready()) => Ok(false), Err(e) => Err(e), } } @@ -308,21 +308,25 @@ impl ScsiTransport for SptiTransport { // surfaces the failure to UX. return Err(Error::ScsiError { opcode: cdb[0], - status: 0xFF, - sense_key: 0, + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }); } if sptwb.spt.ScsiStatus != 0 { // SPTI doesn't surface a "bytes written into sense buffer" // count separate from SenseInfoLength (input). Pass the full - // K_SENSE_SIZE; parse_sense_key keys off byte 0's response - // code to handle descriptor (0x72/0x73) vs fixed (0x70/0x71). - let sense_key = super::parse_sense_key(&sptwb.sense, K_SENSE_SIZE as u8); + // K_SENSE_SIZE; parse_sense keys off byte 0's response code + // to handle descriptor (0x72/0x73) vs fixed (0x70/0x71). + // + // 0.13.23: carry the full SPC-4 sense triple in + // `Error::ScsiError::sense` so callers can route on + // `ScsiSense::is_medium_error()` etc. + let parsed = super::parse_sense(&sptwb.sense, K_SENSE_SIZE as u8); return Err(Error::ScsiError { opcode: cdb[0], status: sptwb.spt.ScsiStatus, - sense_key, + sense: Some(parsed), }); } diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index 82a7997..583beb0 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -392,7 +392,7 @@ impl FailingSectorReader { impl SectorReader for FailingSectorReader { fn read_sectors( &mut self, - lba: u32, + _lba: u32, _count: u16, _buf: &mut [u8], _recovery: bool, @@ -400,7 +400,20 @@ impl SectorReader for FailingSectorReader { if let Some(h) = self.halt_on_first_read.take() { h.store(true, Ordering::Relaxed); } - Err(libfreemkv::error::Error::DiscRead { sector: lba as u64 }) + // Model what a real damaged-disc read returns: CHECK CONDITION + + // MEDIUM ERROR (sense_key 3, ASC 0x11 UNRECOVERED READ ERROR, + // ASCQ 0x05 L-EC UNCORRECTABLE). Disc::copy's hysteresis must + // engage on this — `Error::DiscRead` is libfreemkv's own + // post-classification signal, not what a real reader emits. + Err(libfreemkv::error::Error::ScsiError { + opcode: libfreemkv::scsi::SCSI_READ_10, + status: libfreemkv::scsi::SCSI_STATUS_CHECK_CONDITION, + sense: Some(libfreemkv::ScsiSense { + sense_key: libfreemkv::scsi::SENSE_KEY_MEDIUM_ERROR, + asc: 0x11, + ascq: 0x05, + }), + }) } fn capacity(&self) -> u32 { @@ -570,7 +583,18 @@ impl SectorReader for BlockSizeFailingReader { } Ok(buf.len()) } else { - Err(libfreemkv::error::Error::DiscRead { sector: lba as u64 }) + // Multi-sector reads fail with the BU40N's signature: CHECK + // CONDITION + MEDIUM ERROR. The hysteresis must dispatch on + // this as marginal-read and drop to bpt=1. + Err(libfreemkv::error::Error::ScsiError { + opcode: libfreemkv::scsi::SCSI_READ_10, + status: libfreemkv::scsi::SCSI_STATUS_CHECK_CONDITION, + sense: Some(libfreemkv::ScsiSense { + sense_key: libfreemkv::scsi::SENSE_KEY_MEDIUM_ERROR, + asc: 0x11, + ascq: 0x00, + }), + }) } } diff --git a/tests/scsi_error_decoding.rs b/tests/scsi_error_decoding.rs index 8657360..d86dcdf 100644 --- a/tests/scsi_error_decoding.rs +++ b/tests/scsi_error_decoding.rs @@ -1,37 +1,55 @@ //! Integration tests for the SCSI error-decoding contract. //! //! v0.13.20 rewrote `scsi/linux.rs` to a synchronous blocking SG_IO and -//! consolidated sense-key parsing into the `parse_sense_key` helper that -//! every platform backend now shares. The actual `ioctl(SG_IO, ...)` call -//! is impossible to mock without a kernel — see -//! `(internal)/docs/audits/2026-04-26-scsi-architecture-research.md` -//! for why the audit recommends against libc shims here. +//! consolidated sense parsing into the `parse_sense` helper that every +//! platform backend now shares. v0.13.23 replaced the `Error::ScsiError` +//! flat-fields shape with `{ opcode, status, sense: Option }` +//! so callers can route on structured sense data (key + ASC + ASCQ) via +//! [`Error::scsi_sense`] / [`Error::is_marginal_read`] / +//! [`ScsiSense::is_*`]. //! -//! These tests pin the *contract* every backend must satisfy: +//! The actual `ioctl(SG_IO, ...)` call is impossible to mock without a +//! kernel — see +//! `(internal)/docs/audits/2026-04-26-scsi-architecture-research.md` +//! for why the audit recommends against libc shims here. These tests +//! therefore pin the *contract* every backend must satisfy via a mock +//! `ScsiTransport`: //! //! 1. Healthy result → `Ok(ScsiResult { bytes_transferred = data.len() - resid })`. -//! 2. Transport-level failure (`host_status` or `driver_status` non-zero) -//! → `Error::ScsiError { status: 0xFF, sense_key: 0 }`. Used by +//! 2. Transport-level failure (no SCSI status delivered: kernel +//! timeout, USB bridge wedge, IOKit service error) → +//! `Error::ScsiError { status: SCSI_STATUS_TRANSPORT_FAILURE, sense: None }`. +//! `Error::is_scsi_transport_failure()` returns `true`. Used by //! `drive_has_disc` to detect the wedge signature. -//! 3. SCSI-level failure (status non-zero, sense buffer populated) → -//! `Error::ScsiError { status, sense_key }` with the parsed key. -//! 4. Sense-key parsing handles descriptor (0x72/0x73) and fixed -//! (0x70/0x71) response codes; missing sense data → key 0. +//! 3. SCSI-level failure (drive replied CHECK CONDITION with sense) → +//! `Error::ScsiError { status: 0x02, sense: Some(ScsiSense {…}) }` +//! with the parsed key/ASC/ASCQ. +//! 4. `Error::is_marginal_read()` is `true` for MEDIUM ERROR / +//! ABORTED COMMAND / RECOVERED ERROR / NO SENSE; `false` for +//! HARDWARE / DATA PROTECT / UNIT ATTENTION / NOT READY / ILLEGAL +//! REQUEST and for transport failures. //! -//! The mock `ScsiTransport` here emulates exactly that layered shape. //! Inline `parse_sense_tests` in `src/scsi/mod.rs` cover the pure parse -//! logic; this file covers the consumer side — a real transport feeding -//! a real Error variant to a real call site (`scsi::inquiry`). +//! logic (descriptor 0x72/0x73 vs fixed 0x70/0x71, short-buffer, VALID +//! bit masking, unknown response codes, ASC/ASCQ offsets); this file +//! covers the consumer side — a real transport feeding a real Error +//! variant to a real call site (`scsi::inquiry`). use libfreemkv::error::Error; -use libfreemkv::scsi::{DataDirection, ScsiResult, ScsiTransport}; +use libfreemkv::scsi::{ + DataDirection, SCSI_STATUS_CHECK_CONDITION, SCSI_STATUS_TRANSPORT_FAILURE, ScsiResult, + ScsiSense, ScsiTransport, SENSE_KEY_ABORTED_COMMAND, SENSE_KEY_DATA_PROTECT, + SENSE_KEY_HARDWARE_ERROR, SENSE_KEY_ILLEGAL_REQUEST, SENSE_KEY_MEDIUM_ERROR, + SENSE_KEY_NOT_READY, SENSE_KEY_RECOVERED_ERROR, SENSE_KEY_UNIT_ATTENTION, +}; /// A scripted ScsiTransport. Each `execute()` consumes the next entry /// from `script` and returns the corresponding outcome. /// /// Outcomes mirror what each backend's `execute()` should produce after -/// the v0.13.20 rewrite: pre-parsed sense_key and synthesized 0xFF -/// status for transport-level failures. +/// the v0.13.23 sense plumbing: `Option` carrying the full +/// SPC-4 triple for drive-reported failures, `None` for transport-level +/// failures. struct MockTransport { script: Vec, next: usize, @@ -46,16 +64,16 @@ enum MockOutcome { data: Vec, resid: i32, }, - /// Transport-level failure: e.g. `hdr.host_status = DID_TIME_OUT` - /// on Linux, or `kIOReturnError` on macOS, or `DeviceIoControl` - /// returning 0 on Windows. Backends synthesize 0xFF. + /// Transport-level failure: `hdr.host_status = DID_TIME_OUT` on + /// Linux, `kIOReturnError` on macOS, `DeviceIoControl` returning 0 + /// on Windows. Backends synthesise `SCSI_STATUS_TRANSPORT_FAILURE` + /// with `sense = None`. TransportFailure, - /// SCSI-level failure: device responded with a non-zero status and - /// some sense data. `status` and `sense_key` are what the caller - /// must see on the `Error::ScsiError` variant. + /// Drive replied with sense data (typically `SCSI_STATUS_CHECK_CONDITION` + /// + a populated sense buffer). ScsiFailure { status: u8, - sense_key: u8, + sense: ScsiSense, }, } @@ -93,18 +111,31 @@ impl ScsiTransport for MockTransport { } MockOutcome::TransportFailure => Err(Error::ScsiError { opcode: cdb[0], - status: 0xFF, - sense_key: 0, + status: SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, }), - MockOutcome::ScsiFailure { status, sense_key } => Err(Error::ScsiError { + MockOutcome::ScsiFailure { status, sense } => Err(Error::ScsiError { opcode: cdb[0], status, - sense_key, + sense: Some(sense), }), } } } +/// Helper — build a CHECK CONDITION outcome with a given sense key +/// and zero ASC/ASCQ. Most consumer-side tests only care about the key. +fn check_cond(sense_key: u8) -> MockOutcome { + MockOutcome::ScsiFailure { + status: SCSI_STATUS_CHECK_CONDITION, + sense: ScsiSense { + sense_key, + asc: 0, + ascq: 0, + }, + } +} + // ── 1. Healthy read ─────────────────────────────────────────────────────── #[test] @@ -127,111 +158,167 @@ fn test_healthy_inquiry_returns_ok_with_full_transfer() { assert_eq!(r.firmware, "1.00"); } -// ── 2. Transport-level failure (host_status or driver_status non-zero) ───── -// -// Linux: kernel sets `hdr.host_status = DID_TIME_OUT (0x03)` or -// `hdr.driver_status` non-zero on a USB bridge wedge. SgIoTransport -// synthesizes ScsiError { status: 0xFF, sense_key: 0 }. -// macOS: IOKit `ExecuteTaskSync` returns non-zero IOReturn; same shape. -// Windows: `DeviceIoControl` returns 0; same shape. -// -// Callers (drive_has_disc, etc.) match on status == 0xFF as the wedge -// signature. This test pins that contract. +// ── 2. Transport-level failure: no SCSI status, no sense data ───────────── #[test] -fn test_transport_failure_surfaces_as_status_0xff_sense_key_0() { +fn test_transport_failure_surfaces_with_sense_none() { let mut transport = MockTransport::new(vec![MockOutcome::TransportFailure]); let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + assert!( + err.is_scsi_transport_failure(), + "TransportFailure must satisfy is_scsi_transport_failure()" + ); + assert!( + err.scsi_sense().is_none(), + "transport failure has no sense data" + ); + assert!( + !err.is_marginal_read(), + "transport failure must not be classified as marginal-read" + ); match err { - Error::ScsiError { - status, sense_key, .. - } => { - assert_eq!(status, 0xFF, "transport failure must surface as 0xFF"); - assert_eq!(sense_key, 0, "transport failure has no sense key"); + Error::ScsiError { status, sense, .. } => { + assert_eq!(status, SCSI_STATUS_TRANSPORT_FAILURE); + assert!(sense.is_none()); } other => panic!("expected ScsiError, got {other:?}"), } } -// ── 3. SCSI-level failure with descriptor-format sense (0x72) ───────────── +// ── 3. CHECK CONDITION + ILLEGAL REQUEST ────────────────────────────────── #[test] -fn test_scsi_failure_descriptor_format_illegal_request() { - // A real device returning CHECK CONDITION (status 0x02) with - // descriptor-format sense indicating ILLEGAL REQUEST (key 5). - // The Linux backend's parse_sense_key reads byte 1; the caller sees - // sense_key = 5. - let mut transport = MockTransport::new(vec![MockOutcome::ScsiFailure { - status: 0x02, - sense_key: 5, - }]); - +fn test_check_cond_illegal_request_carries_sense() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_ILLEGAL_REQUEST)]); let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); - match err { - Error::ScsiError { - opcode, - status, - sense_key, - } => { - assert_eq!(opcode, libfreemkv::scsi::SCSI_INQUIRY); - assert_eq!(status, 0x02); - assert_eq!(sense_key, 5); - } - other => panic!("expected ScsiError, got {other:?}"), - } + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert_eq!(sense.sense_key, SENSE_KEY_ILLEGAL_REQUEST); + assert!(sense.is_illegal_request()); + assert!(!err.is_marginal_read(), "ILLEGAL REQUEST is not marginal"); + assert!(!err.is_scsi_transport_failure()); } -// ── 4. SCSI-level failure with NOT READY sense ─────────────────────────── -// -// `drive_has_disc` matches on sense_key 2 to mean "no disc inserted" -// rather than a hard error. Pin that contract via the consumer. +// ── 4. CHECK CONDITION + NOT READY (drive_has_disc relies on this) ──────── #[test] -fn test_scsi_failure_not_ready_key_2_propagates_intact() { - let mut transport = MockTransport::new(vec![MockOutcome::ScsiFailure { - status: 0x02, - sense_key: 2, - }]); - +fn test_check_cond_not_ready_predicate() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_NOT_READY)]); let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); - match err { - Error::ScsiError { - status, sense_key, .. - } => { - assert_eq!(status, 0x02); - assert_eq!(sense_key, 2); - } - other => panic!("expected ScsiError, got {other:?}"), - } + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert!(sense.is_not_ready(), "sense_key 2 ⇒ is_not_ready"); + assert!(!err.is_marginal_read(), "NOT READY is not marginal"); } -// ── 5. SCSI-level failure with empty sense ─────────────────────────────── -// -// Backends pass `sb_len_wr` to `parse_sense_key`; when zero, the helper -// returns 0. From the caller's perspective this is `sense_key = 0` -// (NO SENSE) on a non-zero status — surface that contract. +// ── 5. CHECK CONDITION + MEDIUM ERROR (canonical marginal-read) ─────────── #[test] -fn test_scsi_failure_empty_sense_returns_key_0() { - let mut transport = MockTransport::new(vec![MockOutcome::ScsiFailure { - status: 0x02, - sense_key: 0, - }]); - +fn test_check_cond_medium_error_is_marginal() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_MEDIUM_ERROR)]); let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); - match err { - Error::ScsiError { - status, sense_key, .. - } => { - assert_eq!(status, 0x02); - assert_eq!(sense_key, 0); - } - other => panic!("expected ScsiError, got {other:?}"), - } + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert!(sense.is_medium_error()); + assert!(sense.is_marginal()); + assert!( + err.is_marginal_read(), + "MEDIUM ERROR is the canonical marginal-read signal" + ); } -// ── 6. Healthy short transfer (resid > 0) ──────────────────────────────── +// ── 6. CHECK CONDITION + ABORTED COMMAND (also marginal) ────────────────── + +#[test] +fn test_check_cond_aborted_command_is_marginal() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_ABORTED_COMMAND)]); + let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert!(sense.is_aborted_command()); + assert!(err.is_marginal_read(), "ABORTED COMMAND is marginal"); +} + +// ── 7. CHECK CONDITION + RECOVERED ERROR (drive recovered; marginal) ────── + +#[test] +fn test_check_cond_recovered_error_is_marginal() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_RECOVERED_ERROR)]); + let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + assert!( + err.is_marginal_read(), + "RECOVERED ERROR is treated as marginal (drive recovered, retry-friendly class)" + ); +} + +// ── 8. CHECK CONDITION + HARDWARE ERROR (NOT marginal — bail) ───────────── + +#[test] +fn test_check_cond_hardware_error_not_marginal() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_HARDWARE_ERROR)]); + let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert!(sense.is_hardware_error()); + assert!( + !err.is_marginal_read(), + "HARDWARE ERROR must not be marginal — drive failing, retry can't help" + ); +} + +// ── 9. CHECK CONDITION + DATA PROTECT (NOT marginal — bail) ─────────────── + +#[test] +fn test_check_cond_data_protect_not_marginal() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_DATA_PROTECT)]); + let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert!(sense.is_data_protect()); + assert!( + !err.is_marginal_read(), + "DATA PROTECT (AACS / region) must not be marginal — retry can't help" + ); +} + +// ── 10. CHECK CONDITION + UNIT ATTENTION (NOT marginal — caller rescans) ── + +#[test] +fn test_check_cond_unit_attention_not_marginal() { + let mut transport = MockTransport::new(vec![check_cond(SENSE_KEY_UNIT_ATTENTION)]); + let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + + let sense = err.scsi_sense().expect("CHECK CONDITION must carry sense"); + assert!(sense.is_unit_attention()); + assert!( + !err.is_marginal_read(), + "UNIT ATTENTION must not be marginal — caller should rescan, not retry" + ); +} + +// ── 11. ASC/ASCQ propagate from sense buffer to ScsiError.sense ─────────── + +#[test] +fn test_asc_ascq_round_trip_through_error() { + let outcome = MockOutcome::ScsiFailure { + status: SCSI_STATUS_CHECK_CONDITION, + sense: ScsiSense { + sense_key: SENSE_KEY_MEDIUM_ERROR, + asc: 0x11, + ascq: 0x05, // L-EC UNCORRECTABLE + }, + }; + let mut transport = MockTransport::new(vec![outcome]); + let err = libfreemkv::scsi::inquiry(&mut transport).unwrap_err(); + + let sense = err.scsi_sense().expect("must carry sense"); + assert_eq!(sense.sense_key, SENSE_KEY_MEDIUM_ERROR); + assert_eq!(sense.asc, 0x11); + assert_eq!(sense.ascq, 0x05); +} + +// ── 12. Healthy short transfer (resid > 0) ──────────────────────────────── #[test] fn test_healthy_short_transfer_reports_partial_bytes() { @@ -243,9 +330,6 @@ fn test_healthy_short_transfer_reports_partial_bytes() { resid: 16, }]); - // Drive INQUIRY through the public helper to exercise the consumer - // path; INQUIRY itself doesn't act on bytes_transferred but the - // transport contract is what we care about. let cdb = [libfreemkv::scsi::SCSI_INQUIRY, 0, 0, 0, 0x60, 0]; let mut buf = [0u8; 96]; let r = transport @@ -258,25 +342,28 @@ fn test_healthy_short_transfer_reports_partial_bytes() { ); } -// ── 7. Error::Display does not leak English in the SCSI variant ─────────── +// ── 13. Error::Display does not leak English in the SCSI variant ────────── // // Library rule (project docs): no English in error display, only "E{code}" -// + structured data. Existing error-mod tests cover the new variants; -// this is a regression guard for the SCSI variant specifically because -// it's the most-emitted error in the rip path. +// + structured data. Regression guard for the SCSI variant specifically +// because it's the most-emitted error in the rip path. #[test] fn test_scsi_error_display_format_is_codes_only() { let err = Error::ScsiError { opcode: 0x12, - status: 0x02, - sense_key: 5, + status: SCSI_STATUS_CHECK_CONDITION, + sense: Some(ScsiSense { + sense_key: SENSE_KEY_ILLEGAL_REQUEST, + asc: 0x24, + ascq: 0x00, + }), }; let s = err.to_string(); assert!(s.starts_with("E4000:"), "ScsiError must lead with E4000: {s}"); assert!( - s.contains("0x12") && s.contains("0x02") && s.contains("0x05"), - "ScsiError must show opcode/status/sense_key in hex: {s}" + s.contains("0x12") && s.contains("0x02") && s.contains("0x05") && s.contains("0x24"), + "ScsiError must show opcode/status/key/asc in hex: {s}" ); // Crude English filter — same as the inline error.rs::display test. for word in s.split(|c: char| !c.is_ascii_alphabetic()) { @@ -286,3 +373,22 @@ fn test_scsi_error_display_format_is_codes_only() { ); } } + +// ── 14. Transport-failure Display omits sense fields ────────────────────── + +#[test] +fn test_scsi_transport_failure_display_short_form() { + let err = Error::ScsiError { + opcode: 0x28, + status: SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, + }; + let s = err.to_string(); + assert!(s.starts_with("E4000:")); + assert!(s.contains("0x28") && s.contains("0xff")); + // No sense triple should appear in the no-sense form. + assert!( + !s.contains("0x00/0x00/0x00"), + "transport failure must not carry phantom sense: {s}" + ); +}