From 05fed1b0e0b5b7da61da2e4bcd9267ebee0f9c96 Mon Sep 17 00:00:00 2001
From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com>
Date: Thu, 30 Jul 2026 08:42:02 -0700
Subject: [PATCH] Log the OS error when a SCSI command fails on Windows and
 macOS
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both backends discarded the platform's own error code on the execute hot
path — the one every READ(10) of a rip goes through — and collapsed every
cause to the same status-0xFF transport failure.

On Windows, open() and reset() in the same file both capture the Win32
error; execute() did not. That left ERROR_INVALID_PARAMETER (a struct
layout regression, the exact class this file's SDK-layout tests exist to
catch), ERROR_ACCESS_DENIED and ERROR_GEN_FAILURE (a genuinely wedged
drive) indistinguishable, with nothing in the log to tell a code bug from
a hardware one.

On macOS the same, and worse: the file had no tracing calls at all, where
the Linux and Windows backends both log their execute failures. Its open()
carefully decodes the shim's sentinel into typed variants instead of
flattening them, but execute() threw the IOKit return away — so another
process taking exclusive access mid-rip and a real hardware wedge produced
identical, empty diagnostics.

Logged rather than added to the error type: the typed variant is public
API, and the recovery classification is deliberately the same for all of
these. What was missing is the breadcrumb, not the distinction.

Two further findings from the same sweep were rejected. Windows reset()
always returning Ok(()) and macOS ignoring timeout_ms are both already
documented in the code as deliberate, and the reporter flagged them for
completeness rather than as defects.

Neither fix has a test: reaching either branch needs a failing ioctl or a
failing IOKit call, and both files are compiled only on their own platform.
---
 src/scsi/macos.rs   | 14 ++++++++++++++
 src/scsi/windows.rs | 18 ++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/src/scsi/macos.rs b/src/scsi/macos.rs
index d776ec7..861563f 100644
--- a/src/scsi/macos.rs
+++ b/src/scsi/macos.rs
@@ -181,6 +181,20 @@ impl ScsiTransport for MacScsiTransport {
         };
 
         if kr != 0 {
+            // Log the IOKit return before collapsing it. `open()` above decodes
+            // the shim's sentinel into typed variants rather than flattening
+            // every failure, but `execute()` discarded `kr` entirely — and this
+            // file had no tracing at all, where the Linux and Windows backends
+            // both log their execute failures. That left resource contention
+            // (another process taking exclusive access mid-rip) and a real
+            // hardware wedge indistinguishable, with no diagnostic trail on
+            // either.
+            tracing::warn!(
+                target: "freemkv::scsi",
+                opcode = cdb.first().copied().unwrap_or(0),
+                kr,
+                "shim_execute failed"
+            );
             return Err(Error::ScsiError {
                 opcode: cdb.first().copied().unwrap_or(0),
                 status: super::SCSI_STATUS_TRANSPORT_FAILURE,
diff --git a/src/scsi/windows.rs b/src/scsi/windows.rs
index b910874..25082e8 100644
--- a/src/scsi/windows.rs
+++ b/src/scsi/windows.rs
@@ -531,6 +531,24 @@ impl ScsiTransport for SptiTransport {
         };
 
         if ok == 0 {
+            // Capture the Win32 error before returning. `open()` and `reset()`
+            // in this file both do; `execute()` did not, and it is the hot path
+            // — every READ(10) of a rip goes through here. Without it
+            // ERROR_INVALID_PARAMETER (a struct-layout regression, the very
+            // class this file's SDK-layout tests exist to catch),
+            // ERROR_ACCESS_DENIED and ERROR_GEN_FAILURE (a genuinely wedged
+            // drive) all collapse to the same status-0xFF transport failure
+            // with nothing in the log to tell a code bug from a hardware one.
+            // Logged rather than added to the error type: the typed variant is
+            // public API and the recovery classification is deliberately the
+            // same for all of them.
+            let last_error = unsafe { GetLastError() };
+            tracing::warn!(
+                target: "freemkv::scsi",
+                opcode = cdb.first().copied().unwrap_or(0),
+                last_error,
+                "DeviceIoControl(IOCTL_SCSI_PASS_THROUGH_DIRECT) failed"
+            );
             // Driver-level failure (timeout, handle gone, etc.). Bubble
             // up; in-library handle recovery was removed in 0.13.20 along
             // with Linux's async fd-recovery and macOS's `try_recover` —