fix(ecstore): resolve nine unused bindings in set_disk write and heal paths (#6158)

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).
This commit is contained in:
Zhengchao An
2026-08-17 08:06:37 +08:00
committed by GitHub
parent 33cd11472a
commit 9f02ca6c36
4 changed files with 17 additions and 13 deletions
@@ -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, Ok(collected) => collected,
Err(()) => return empty_quorum_result(), Err(()) => return empty_quorum_result(),
}; };
// debug!("ReadMultipleResp ress {:?}", ress);
// debug!("ReadMultipleResp errors {:?}", errors);
let mut ret = Vec::with_capacity(req.files.len()); let mut ret = Vec::with_capacity(req.files.len());
for want in req.files.iter() { for want in req.files.iter() {
+7 -2
View File
@@ -453,7 +453,9 @@ impl SetDisks {
..Default::default() ..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?; let ns_lock = self.new_ns_lock(bucket, object).await?;
Some( Some(
ns_lock ns_lock
@@ -996,7 +998,7 @@ impl SetDisks {
readers.push(None); readers.push(None);
continue; continue;
} }
Err(e) => { Err(_e) => {
readers.push(None); readers.push(None);
continue; continue;
} }
@@ -1545,6 +1547,9 @@ impl SetDisks {
for candidate in candidates.iter_mut().filter(|candidate| candidate.local_payload) { for candidate in candidates.iter_mut().filter(|candidate| candidate.local_payload) {
for (disk_index, disk) in disks.iter().enumerate() { 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 { let Some(disk) = disk else {
return Ok(DanglingDeleteSafety::UnsafeToDelete); return Ok(DanglingDeleteSafety::UnsafeToDelete);
}; };
+5 -5
View File
@@ -1400,7 +1400,7 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks {
let mut count = max_parts; 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 { if let Some(err) = &part.error {
warn!("list_object_parts part error: {:?}", &err); 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) && let Err(err) = checksum.add_part(&cs, ext_part.actual_size)
{ {
error!( error!(
"complete_multipart_upload checksum add_part failed part_id={}, bucket={}, object={}", "complete_multipart_upload checksum add_part failed part_id={}, bucket={}, object={}, err={}",
p.part_num, bucket, object p.part_num, bucket, object, err
); );
return Err(Error::InvalidPart(p.part_num, ext_part.etag.clone(), p.etag.clone().unwrap_or_default())); 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) { } else if let Err(err) = wtcs.matches(&checksum_combined, uploaded_parts.len() as i32) {
error!( error!(
"complete_multipart_upload checksum matches failed want={}, got={}", "complete_multipart_upload checksum matches failed want={}, got={}, err={}",
wtcs.encoded, checksum.encoded wtcs.encoded, checksum.encoded, err
); );
return Err(Error::other(format!( return Err(Error::other(format!(
"complete_multipart_upload checksum matches failed want={}, got={}", "complete_multipart_upload checksum matches failed want={}, got={}",
+4 -2
View File
@@ -5656,7 +5656,9 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks {
// TODO: Lifecycle // TODO: Lifecycle
let mut version_found = true; 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 if let Some(err) = &gerr
&& goi.name.is_empty() && 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); self.record_capacity_scope_if_needed(opts.capacity_scope_token, &disks);
for disk in disks.iter() { for disk in disks.iter() {
if let Some(disk) = disk { if disk.is_some() {
continue; continue;
} }
let _ = self let _ = self