fix(extract): read libarchive's error after the write, not before - #126
Junker der Provinz (junkerderprovinz) wants to merge 1 commit into
Conversation
The four archive_write_disk() call sites in extractMultiFileRun() passed the write result and archive_error_string(ext) as two arguments of a single requireArchiveWriteSuccess() call. C++ leaves the evaluation order of function arguments unspecified, and GCC, which is what the Windows build uses (win64_mingw / tools_mingw1310), evaluates them right to left. The error slot was therefore read before the write ran. Since archive_write_header() had already cleared it for that entry, the read always returned NULL and every write failure surfaced as the generic "Failed to write a file to the target drive" fallback, discarding libarchive's own message. Sequence each write into its own statement and hand the already known result to a new _requireWriteSucceeded() helper, which logs the result, archive_errno(ext) and libarchive's message through qWarning() before turning it into an exception. archive_errno() carries the Win32 code that libarchive's la_dosmaperr() mapped from GetLastError(), which is the value needed to name the real cause of a failed write, and qWarning() is the path --log-file captures. The entry-header warning a few lines above now logs archive_errno() as well, so both paths report the same detail. archive_write_result.h is deliberately left alone: it is unit tested in isolation and must not grow a Qt or libarchive dependency. Fixes unraid#122
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe archive extraction path now validates libarchive writes after each operation. It logs operation-specific failure details, including pathname, errno, and libarchive messages. Entry-header diagnostics also include errno. ChangesArchive write diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This is a localized fix that preserves the existing archive-write flow while surfacing the underlying write error; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Junker der Provinz (junkerderprovinz)
changed the title
1 participant
Fixes #122.
The bug
extractMultiFileRun()had four call sites shaped like:Unraid::requireArchiveWriteSuccess(batcher.Add(buff, size, offset), archive_error_string(ext));C++ leaves the evaluation order of function arguments unspecified, and this project's Windows build uses MinGW GCC (
win64_mingw/tools_mingw1310in.github/workflows/build.yml), which evaluates them right to left. That meansarchive_error_string(ext)was read before the write ran, andarchive_write_header()had already cleared the error slot for that entry a few lines earlier, so the read always returned NULL. Every failure at these four sites therefore fell back to the generic"Failed to write a file to the target drive"message and lostarchive_errno(), which is the actual Win32-mapped error code, exactly what #122 says it couldn't capture.The header-check path a few lines above does this correctly today, because
ris assigned as its own statement first — that's the tell that this was an oversight on the write path specifically, not a deliberate choice.The fix
At all four sites, the call that can fail is computed into a named local as its own statement, so the compiler can no longer observably reorder anything, and a new small helper (
_requireWriteSucceeded, beside the existing_checkResultin the same file) logsarchive_error_string(ext)andarchive_errno(ext)viaqWarning()(so--log-filecaptures it, same mechanism the header path already uses) before delegating to the existing, unmodifiedUnraid::requireArchiveWriteSuccess().archive_write_result.hitself is left untouched on purpose. It's dependency-free and has its own standalone unit test target that doesn't link Qt or libarchive; extending its signature to carry the errno would have dragged both into that test.Two small things beyond the four sites, both deliberate, flagged here so they're a decision rather than a surprise:
tr()wrapper one level up, only the wrapper is translated) — it's the actual fix, just visible in the UI too.qWarning()also gainedarchive_errno()logging. Same failure family, same function, one line — leaving it out would mean the header path is still the one place that can't name its Win32 cause.Verification
I could not build this locally: the environment I had available is GCC only, with no Qt6 and no CMake, and libarchive is fetched at configure time, so even a syntax-only check of the real translation unit was out of reach.
Instead I verified the change against a harness that includes the real
block_batcher.handarchive_write_result.h, uses theARCHIVE_*values from libarchive v3.8.7 as pinned insrc/dependencies/libarchive.cmake, and compiles the new helper's exact committed text (extracted directly from the file, not retyped). That harness reproduces the old behavior (generic fallback message, errno lost) and the new one (libarchive's own message plus errno) at all four call sites, confirms the log predicate and the throw predicate agree for every result value in range, confirms the null-pathname path, and independently confirms the underlying GCC argument-evaluation-order mechanism directly (read-before-write at both -O0 and -O2). It also confirmsqWarning() << (pathname ? pathname : "target drive")resolves to theconst char*overload, not something surprising, by reconstructing Qt's actualQDebugoverload set.What it does not do is compile the real translation unit against real Qt6/libarchive, so CI is the first genuine build of this change on Windows, macOS and Linux.
Summary by CodeRabbit