[pbs-devel] [PATCH proxmox-backup 2/2] 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', which maps anyhow Errors as type 'Tape' and
io Errors as type 'Disk'

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/restore.rs | 97 ++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 43 deletions(-)

diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 9f79f06f..8058765d 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -18,6 +18,7 @@ use proxmox::{
         schema::parse_property_string,
         section_config::SectionConfigData,
     },
+    io_format_err,
     tools::{
         Uuid,
         io::ReadExt,
@@ -646,6 +647,16 @@ 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(#[from] std::io::Error),
+}
+
 fn restore_snapshot_archive<'a>(
     worker: &WorkerTask,
     reader: Box<dyn 'a + TapeRead>,
@@ -655,7 +666,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 +682,25 @@ fn restore_snapshot_archive<'a>(
             // else the archive is corrupt
             Err(err)
         }
+        Err(RestoreError::Disk(err)) => Err(err.into()),
     }
 }
 
+// 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 = decoder
+        .next()
+        .ok_or_else(|| format_err!("missing root entry"))?
+        .map_err(Error::from)?;
+
+    if !matches!(root.kind(), pxar::EntryKind::Directory) {
+        return Err(format_err!("wrong root entry type").into());
+    }
 
     let root_path = Path::new("/");
     let manifest_file_name = OsStr::new(MANIFEST_BLOB_NAME);
@@ -702,29 +712,27 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
 
         let entry = match decoder.next() {
             None => break,
-            Some(entry) => entry?,
+            Some(entry) => entry.map_err(Error::from)?,
         };
+
         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(format_err!("wrong entry type for {:?}", entry_path).into());
         }
-        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(|| format_err!("wrong parent for {:?}", entry_path))?;
+
+        if path != root_path {
+            return Err(format_err!("wrong parent for {:?}", entry_path).into());
         }
 
         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(|| format_err!("missing file content"))?;
         let mut archive_path = snapshot_path.to_owned();
         archive_path.push(&filename);
 
@@ -733,31 +741,30 @@ 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(Error::from)?;
             let options = CreateOptions::new();
             replace_file(&tmp_path, blob.raw_data(), options)?;
 
-            manifest = Some(BackupManifest::try_from(blob)?);
+            manifest = Some(BackupManifest::try_from(blob).map_err(Error::from)?);
         } 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))?;
+                .map_err(|err| io_format_err!("restore {:?} failed - {}", tmp_path, err))?;
 
-            std::io::copy(&mut contents, &mut tmpfile)?;
+            // 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(Error::from)?;
 
-            if let Err(err) = std::fs::rename(&tmp_path, &archive_path) {
-                bail!("Atomic rename file {:?} failed - {}", archive_path, err);
-            }
+            std::fs::rename(&tmp_path, &archive_path).map_err(|err| {
+                io_format_err!("Atomic rename file {:?} failed - {}", archive_path, err)
+            })?;
         }
     }
 
-    let manifest = match manifest {
-        None => bail!("missing manifest"),
-        Some(manifest) => manifest,
-    };
+    let manifest = manifest.ok_or_else(|| format_err!("missing manifest"))?;
 
     for item in manifest.files() {
         let mut archive_path = snapshot_path.to_owned();
@@ -788,9 +795,13 @@ 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| {
+        io_format_err!(
+            "Atomic rename manifest {:?} failed - {}",
+            manifest_path,
+            err
+        )
+    })?;
 
     Ok(())
 }
-- 
2.20.1