Log the OS error when a SCSI command fails on Windows and macOS
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.
This commit is contained in:
@@ -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` —
|
||||
|
||||
Reference in New Issue
Block a user