From c8e7bb0fca5e476e4f3b01795f5079a95b831cb8 Mon Sep 17 00:00:00 2001 From: overtrue Date: Mon, 17 Aug 2026 01:53:16 +0800 Subject: [PATCH] fix(ecstore): resolve nine unused bindings in set_disk write and heal paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit backlog#1823 step 1, the diagnosis half. Temporarily removing set_disk/mod.rs's #![allow(unused_variables)] surfaced nine bindings. The issue asks that values computed and then dropped on write/quorum paths be diagnosed before being underscored, and that turned out to matter: only four were plain leftovers. Two errors were bound and then left out of the log they were bound for. complete_multipart_upload's checksum failures read `if let Err(err) = ...` and then log part_id, bucket and object with no `err` anywhere in the message, so a checksum failure in production told you which part failed but not why. Both messages now carry the error. One is a lock guard. heal's write_lock_guard holds a namespace write lock for the rest of the scope; renaming it to a bare `_` would drop it immediately and release the lock. It is now `_write_lock_guard`, with a comment saying why it must not be `_`. One was kept alive by a corpse. `errors` in read_multiple_files is read by nothing except two commented-out debug! lines directly below it; the binding and the commented lines go together. One is a cfg split. heal's disk_index is read only inside the #[cfg(test)] fault-injection branch, so underscoring it would break the test build; a `#[cfg(not(test))] let _ = disk_index;` covers the non-test lane instead. The remaining four are genuine leftovers: an unused enumerate index in list_object_parts, a discarded error in a heal reader loop, an inner binding shadowing its own iterator variable, and delete_object's write_quorum. That last one is worth a separate look: delete_object asks get_object_info_and_quorum for a write quorum and never uses it, because delete_object_version below recomputes its own as disks.len() / 2 + 1. The two are not the same number — one comes from the object's erasure configuration, the other is a plain majority of the disk array. Pre-existing behaviour, untouched here. The blankets stay for now. Removing #![allow(unused_imports)] exposes 76 unused imports in set_disk/mod.rs, and they cannot be removed per-lane: cargo fix, working from the lib lane, produced 54 compile errors in the test lane. That needs its own pass with both lanes checked per import. Verification: cargo check -p rustfs-ecstore --tests and --features test-util --tests both warning-free; clippy --lib --tests -D warnings clean; cargo nextest run -p rustfs-ecstore 4101 passed; make pre-commit exit 0. Ref rustfs/backlog#1823 (step 1). --- crates/ecstore/src/set_disk/core/io_primitives.rs | 5 +---- crates/ecstore/src/set_disk/ops/heal.rs | 9 +++++++-- crates/ecstore/src/set_disk/ops/multipart.rs | 10 +++++----- crates/ecstore/src/set_disk/ops/object.rs | 6 ++++-- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index 5f4471f13..41bd6dac7 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -3021,14 +3021,11 @@ impl SetDisks { }); } - let (ress, errors) = match collect_read_multiple_results(futures, read_quorum).await { + let (ress, _errors) = match collect_read_multiple_results(futures, read_quorum).await { Ok(collected) => collected, Err(()) => return empty_quorum_result(), }; - // debug!("ReadMultipleResp ress {:?}", ress); - // debug!("ReadMultipleResp errors {:?}", errors); - let mut ret = Vec::with_capacity(req.files.len()); for want in req.files.iter() { diff --git a/crates/ecstore/src/set_disk/ops/heal.rs b/crates/ecstore/src/set_disk/ops/heal.rs index 24c46a7f5..274b33f43 100644 --- a/crates/ecstore/src/set_disk/ops/heal.rs +++ b/crates/ecstore/src/set_disk/ops/heal.rs @@ -453,7 +453,9 @@ impl SetDisks { ..Default::default() }; - let write_lock_guard = if !opts.no_lock { + // Bound, not `_`: this guard must live to the end of the scope. A bare + // `_` would drop it here and release the namespace write lock. + let _write_lock_guard = if !opts.no_lock { let ns_lock = self.new_ns_lock(bucket, object).await?; Some( ns_lock @@ -996,7 +998,7 @@ impl SetDisks { readers.push(None); continue; } - Err(e) => { + Err(_e) => { readers.push(None); continue; } @@ -1545,6 +1547,9 @@ impl SetDisks { for candidate in candidates.iter_mut().filter(|candidate| candidate.local_payload) { for (disk_index, disk) in disks.iter().enumerate() { + // Only the #[cfg(test)] fault-injection branch below reads this. + #[cfg(not(test))] + let _ = disk_index; let Some(disk) = disk else { return Ok(DanglingDeleteSafety::UnsafeToDelete); }; diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index 86bb41737..35be8062c 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -1400,7 +1400,7 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { let mut count = max_parts; - for (i, part) in object_parts.iter().enumerate() { + for part in object_parts.iter() { if let Some(err) = &part.error { warn!("list_object_parts part error: {:?}", &err); } @@ -2043,8 +2043,8 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { && let Err(err) = checksum.add_part(&cs, ext_part.actual_size) { error!( - "complete_multipart_upload checksum add_part failed part_id={}, bucket={}, object={}", - p.part_num, bucket, object + "complete_multipart_upload checksum add_part failed part_id={}, bucket={}, object={}, err={}", + p.part_num, bucket, object, err ); return Err(Error::InvalidPart(p.part_num, ext_part.etag.clone(), p.etag.clone().unwrap_or_default())); } @@ -2089,8 +2089,8 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { } } else if let Err(err) = wtcs.matches(&checksum_combined, uploaded_parts.len() as i32) { error!( - "complete_multipart_upload checksum matches failed want={}, got={}", - wtcs.encoded, checksum.encoded + "complete_multipart_upload checksum matches failed want={}, got={}, err={}", + wtcs.encoded, checksum.encoded, err ); return Err(Error::other(format!( "complete_multipart_upload checksum matches failed want={}, got={}", diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 670397c12..e7498ee9d 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -5656,7 +5656,9 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { // TODO: Lifecycle let mut version_found = true; - let (mut goi, write_quorum, gerr) = self.get_object_info_and_quorum(bucket, object, &opts).await; + // delete_object_version below derives its own majority quorum from the + // disk array, so the object-derived quorum here is unused. + let (mut goi, _write_quorum, gerr) = self.get_object_info_and_quorum(bucket, object, &opts).await; if let Some(err) = &gerr && goi.name.is_empty() { @@ -6410,7 +6412,7 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { self.record_capacity_scope_if_needed(opts.capacity_scope_token, &disks); for disk in disks.iter() { - if let Some(disk) = disk { + if disk.is_some() { continue; } let _ = self