[pbs-devel] [PATCH proxmox-backup] clean up .bad file handling in sweep_unused_chunks

[pbs-devel] [PATCH proxmox-backup] clean up .bad file handling in sweep_unused_chunks

[Stefan Reiter]

Code cleanup, no functional change intended.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/backup/chunk_store.rs | 58 ++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
index 1d9de70a..7f5c0693 100644
--- a/src/backup/chunk_store.rs
+++ b/src/backup/chunk_store.rs
@@ -325,52 +325,40 @@ impl ChunkStore {
 
             if let Ok(stat) = fstatat(dirfd, filename, nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW) {
                 if bad {
-                    match std::ffi::CString::new(&filename.to_bytes()[..64]) {
-                        Ok(orig_filename) => {
-                            match fstatat(
-                                dirfd,
-                                orig_filename.as_c_str(),
-                                nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW)
-                            {
-                                Ok(_) => { /* do nothing */ },
-                                Err(nix::Error::Sys(nix::errno::Errno::ENOENT)) => {
-                                    // chunk hasn't been rewritten yet, keep
-                                    // .bad file around for manual recovery
-                                    continue;
-                                },
-                                Err(err) => {
-                                    // some other error, warn user and keep
-                                    // .bad file around too
+                    // filename validity checked in iterator
+                    let orig_filename = std::ffi::CString::new(&filename.to_bytes()[..64]).unwrap();
+                    match fstatat(
+                        dirfd,
+                        orig_filename.as_c_str(),
+                        nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW)
+                    {
+                        Ok(_) => {
+                            match unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir) {
+                                Err(err) =>
                                     worker.warn(format!(
-                                        "error during stat on '{:?}' - {}",
-                                        orig_filename,
+                                        "unlinking corrupt chunk {:?} failed on store '{}' - {}",
+                                        filename,
+                                        self.name,
                                         err,
-                                    ));
-                                    continue;
+                                    )),
+                                Ok(_) => {
+                                    status.removed_bad += 1;
+                                    status.removed_bytes += stat.st_size as u64;
                                 }
                             }
                         },
+                        Err(nix::Error::Sys(nix::errno::Errno::ENOENT)) => {
+                            // chunk hasn't been rewritten yet, keep .bad file
+                        },
                         Err(err) => {
+                            // some other error, warn user and keep .bad file around too
                             worker.warn(format!(
-                                "could not get original filename from .bad file '{:?}' - {}",
-                                filename,
+                                "error during stat on '{:?}' - {}",
+                                orig_filename,
                                 err,
                             ));
-                            continue;
                         }
                     }
-
-                    if let Err(err) = unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir) {
-                        worker.warn(format!(
-                            "unlinking corrupt chunk {:?} failed on store '{}' - {}",
-                            filename,
-                            self.name,
-                            err,
-                        ));
-                    } else {
-                        status.removed_bad += 1;
-                        status.removed_bytes += stat.st_size as u64;
-                    }
                 } else if stat.st_atime < min_atime {
                     //let age = now - stat.st_atime;
                     //println!("UNLINK {}  {:?}", age/(3600*24), filename);
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup] clean up .bad file handling in sweep_unused_chunks

[Dietmar Maurer]

> +                    // filename validity checked in iterator
> +                    let orig_filename = std::ffi::CString::new(&filename.to_bytes()[..64]).unwrap();

As discussed in private, CString::new can never fail here, so unwrap() is safe.

Nethertheless, I prefer:

-    let orig_filename = std::ffi::CString::new(&filename.to_bytes()[..64]).unwrap();
+    let orig_filename = std::ffi::CString::new(&filename.to_bytes()[..64])?;

[pbs-devel] applied: [PATCH proxmox-backup] clean up .bad file handling in sweep_unused_chunks

[Dietmar Maurer]

apllied