[pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors

[Dominik Csapak <d.csapak@proxmox.com>]

we sometimes want to ignore tape errors, e.g., if a file was incomplete,
so we must check those conditions only when the error really comes
from the tape side and not from the storage side.

Introduce a RestoreError where we then can decide if we want to handle
tape errors.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* change type of inner of Disk to String (we do not need the exact error)
* add an 'Other' type that we use for worker.check_abort
* make the error types more explicit by using RestoreError::{TYPE} functions
 src/api2/tape/restore.rs | 135 ++++++++++++++++++++++++---------------
 1 file changed, 84 insertions(+), 51 deletions(-)

diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 9f79f06f..b5630524 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -646,6 +646,18 @@ fn restore_chunk_archive<'a>(
     Ok(Some(chunks))
 }
 
+// we want to differentiate between errors that come from disk and tape side,
+// so map anyhow::Error to tape and io::Error to disk
+#[derive(thiserror::Error, Debug)]
+pub enum RestoreError {
+    #[error("tape error: {0}")]
+    Tape(#[from] Error),
+    #[error("{0}")]
+    Disk(String),
+    #[error("{0}")]
+    Other(String),
+}
+
 fn restore_snapshot_archive<'a>(
     worker: &WorkerTask,
     reader: Box<dyn 'a + TapeRead>,
@@ -655,7 +667,7 @@ fn restore_snapshot_archive<'a>(
     let mut decoder = pxar::decoder::sync::Decoder::from_std(reader)?;
     match try_restore_snapshot_archive(worker, &mut decoder, snapshot_path) {
         Ok(()) => Ok(true),
-        Err(err) => {
+        Err(RestoreError::Tape(err)) => {
             let reader = decoder.input();
 
             // check if this stream is marked incomplete
@@ -671,26 +683,28 @@ fn restore_snapshot_archive<'a>(
             // else the archive is corrupt
             Err(err)
         }
+        Err(RestoreError::Other(err)) | Err(RestoreError::Disk(err)) => bail!(err),
     }
 }
 
+// we have to map the errors here according to RestoreError
 fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
     worker: &WorkerTask,
     decoder: &mut pxar::decoder::sync::Decoder<R>,
     snapshot_path: &Path,
-) -> Result<(), Error> {
+) -> Result<(), RestoreError> {
 
-    let _root = match decoder.next() {
-        None => bail!("missing root entry"),
-        Some(root) => {
-            let root = root?;
-            match root.kind() {
-                pxar::EntryKind::Directory => { /* Ok */ }
-                _ => bail!("wrong root entry type"),
-            }
-            root
-        }
-    };
+    let root_res = decoder
+        .next()
+        .ok_or_else(|| RestoreError::Tape(format_err!("missing root entry")))?;
+
+    let root = root_res.map_err(|err| RestoreError::Tape(err.into()))?;
+
+    if !matches!(root.kind(), pxar::EntryKind::Directory) {
+        return Err(RestoreError::Tape(
+            format_err!("wrong root entry type").into(),
+        ));
+    }
 
     let root_path = Path::new("/");
     let manifest_file_name = OsStr::new(MANIFEST_BLOB_NAME);
@@ -698,33 +712,39 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
     let mut manifest = None;
 
     loop {
-        worker.check_abort()?;
+        worker
+            .check_abort()
+            .map_err(|err| RestoreError::Other(format!("{}", err)))?;
 
         let entry = match decoder.next() {
             None => break,
-            Some(entry) => entry?,
+            Some(entry) => entry.map_err(|err| RestoreError::Tape(err.into()))?,
         };
+
         let entry_path = entry.path();
 
-        match entry.kind() {
-            pxar::EntryKind::File { .. } => { /* Ok */ }
-            _ => bail!("wrong entry type for {:?}", entry_path),
+        if !matches!(entry.kind(), pxar::EntryKind::File { .. }) {
+            return Err(RestoreError::Tape(format_err!(
+                "wrong entry type for {:?}",
+                entry_path
+            )));
         }
-        match entry_path.parent() {
-            None => bail!("wrong parent for {:?}", entry_path),
-            Some(p) => {
-                if p != root_path {
-                    bail!("wrong parent for {:?}", entry_path);
-                }
-            }
+
+        let path = entry_path
+            .parent()
+            .ok_or_else(|| RestoreError::Tape(format_err!("wrong parent for {:?}", entry_path)))?;
+
+        if path != root_path {
+            return Err(RestoreError::Tape(format_err!(
+                "wrong parent for {:?}",
+                entry_path
+            )));
         }
 
         let filename = entry.file_name();
-        let mut contents = match decoder.contents() {
-            None => bail!("missing file content"),
-            Some(contents) => contents,
-        };
-
+        let mut contents = decoder
+            .contents()
+            .ok_or_else(|| RestoreError::Tape(format_err!("missing file content")))?;
         let mut archive_path = snapshot_path.to_owned();
         archive_path.push(&filename);
 
@@ -733,31 +753,37 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
 
         if filename == manifest_file_name {
 
-            let blob = DataBlob::load_from_reader(&mut contents)?;
+            let blob = DataBlob::load_from_reader(&mut contents).map_err(RestoreError::Tape)?;
             let options = CreateOptions::new();
-            replace_file(&tmp_path, blob.raw_data(), options)?;
+            replace_file(&tmp_path, blob.raw_data(), options)
+                .map_err(|err| RestoreError::Disk(err.to_string()))?;
 
-            manifest = Some(BackupManifest::try_from(blob)?);
+            manifest = Some(BackupManifest::try_from(blob).map_err(RestoreError::Tape)?);
         } else {
             let mut tmpfile = std::fs::OpenOptions::new()
                 .write(true)
                 .create(true)
                 .read(true)
                 .open(&tmp_path)
-                .map_err(|err| format_err!("restore {:?} failed - {}", tmp_path, err))?;
-
-            std::io::copy(&mut contents, &mut tmpfile)?;
-
-            if let Err(err) = std::fs::rename(&tmp_path, &archive_path) {
-                bail!("Atomic rename file {:?} failed - {}", archive_path, err);
-            }
+                .map_err(|err| {
+                    RestoreError::Disk(format!("restore {:?} failed - {}", tmp_path, err))
+                })?;
+
+            // error type is a bit ambiguous, but err o safe side and assume
+            // a tape error
+            std::io::copy(&mut contents, &mut tmpfile)
+                .map_err(|err| RestoreError::Tape(err.into()))?;
+
+            std::fs::rename(&tmp_path, &archive_path).map_err(|err| {
+                RestoreError::Disk(format!(
+                    "Atomic rename file {:?} failed - {}",
+                    archive_path, err
+                ))
+            })?;
         }
     }
 
-    let manifest = match manifest {
-        None => bail!("missing manifest"),
-        Some(manifest) => manifest,
-    };
+    let manifest = manifest.ok_or_else(|| RestoreError::Tape(format_err!("missing manifest")))?;
 
     for item in manifest.files() {
         let mut archive_path = snapshot_path.to_owned();
@@ -765,18 +791,22 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
 
         match archive_type(&item.filename)? {
             ArchiveType::DynamicIndex => {
-                let index = DynamicIndexReader::open(&archive_path)?;
+                let index = DynamicIndexReader::open(&archive_path)
+                    .map_err(|err| RestoreError::Disk(err.to_string()))?;
                 let (csum, size) = index.compute_csum();
                 manifest.verify_file(&item.filename, &csum, size)?;
             }
             ArchiveType::FixedIndex => {
-                let index = FixedIndexReader::open(&archive_path)?;
+                let index = FixedIndexReader::open(&archive_path)
+                    .map_err(|err| RestoreError::Disk(err.to_string()))?;
                 let (csum, size) = index.compute_csum();
                 manifest.verify_file(&item.filename, &csum, size)?;
             }
             ArchiveType::Blob => {
-                let mut tmpfile = std::fs::File::open(&archive_path)?;
-                let (csum, size) = compute_file_csum(&mut tmpfile)?;
+                let mut tmpfile = std::fs::File::open(&archive_path)
+                    .map_err(|err| RestoreError::Disk(err.to_string()))?;
+                let (csum, size) = compute_file_csum(&mut tmpfile)
+                    .map_err(|err| RestoreError::Disk(err.to_string()))?;
                 manifest.verify_file(&item.filename, &csum, size)?;
             }
         }
@@ -788,9 +818,12 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
     let mut tmp_manifest_path = manifest_path.clone();
     tmp_manifest_path.set_extension("tmp");
 
-    if let Err(err) = std::fs::rename(&tmp_manifest_path, &manifest_path) {
-        bail!("Atomic rename manifest {:?} failed - {}", manifest_path, err);
-    }
+    std::fs::rename(&tmp_manifest_path, &manifest_path).map_err(|err| {
+        RestoreError::Disk(format!(
+            "Atomic rename manifest {:?} failed - {}",
+            manifest_path, err
+        ))
+    })?;
 
     Ok(())
 }
-- 
2.20.1