[pbs-devel] [PATCH proxmox-backup 1/2] api2/tape/restore: restore_chunk_archive: only ignore tape related errors

[pbs-devel] [PATCH proxmox-backup 1/2] api2/tape/restore: restore_chunk_archive: only ignore tape related errors

[Dominik Csapak]

when we get an error from the tape, we possibly want to ignore it,
i.e. when the file was incomplete, but we still want to error
out if the error came from e.g, the datastore, so we have to move
the error checking code to the 'next_chunk' call

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

diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index afce3449..9f79f06f 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -597,54 +597,53 @@ fn restore_chunk_archive<'a>(
 
     let mut decoder = ChunkArchiveDecoder::new(reader);
 
-    let result: Result<_, Error> = proxmox::try_block!({
-        while let Some((digest, blob)) = decoder.next_chunk()? {
-
-            worker.check_abort()?;
-
-            if let Some(datastore) = datastore {
-                let chunk_exists = datastore.cond_touch_chunk(&digest, false)?;
-                if !chunk_exists {
-                    blob.verify_crc()?;
+    loop {
+        let (digest, blob) = match decoder.next_chunk() {
+            Ok(Some((digest, blob))) => (digest, blob),
+            Ok(None) => break,
+            Err(err) => {
+                let reader = decoder.reader();
+
+                // check if this stream is marked incomplete
+                if let Ok(true) = reader.is_incomplete() {
+                    return Ok(Some(chunks));
+                }
 
-                    if blob.crypt_mode()? == CryptMode::None {
-                        blob.decode(None, Some(&digest))?; // verify digest
-                    }
-                    if verbose {
-                        task_log!(worker, "Insert chunk: {}", proxmox::tools::digest_to_hex(&digest));
-                    }
-                    datastore.insert_chunk(&blob, &digest)?;
-                } else if verbose {
-                    task_log!(worker, "Found existing chunk: {}", proxmox::tools::digest_to_hex(&digest));
+                // check if this is an aborted stream without end marker
+                if let Ok(false) = reader.has_end_marker() {
+                    worker.log("missing stream end marker".to_string());
+                    return Ok(None);
                 }
-            } else if verbose {
-                task_log!(worker, "Found chunk: {}", proxmox::tools::digest_to_hex(&digest));
+
+                // else the archive is corrupt
+                return Err(err);
             }
-            chunks.push(digest);
-        }
-        Ok(())
-    });
+        };
 
-    match result {
-        Ok(()) => Ok(Some(chunks)),
-        Err(err) => {
-            let reader = decoder.reader();
+        worker.check_abort()?;
 
-            // check if this stream is marked incomplete
-            if let Ok(true) = reader.is_incomplete() {
-                return Ok(Some(chunks));
-            }
+        if let Some(datastore) = datastore {
+            let chunk_exists = datastore.cond_touch_chunk(&digest, false)?;
+            if !chunk_exists {
+                blob.verify_crc()?;
 
-            // check if this is an aborted stream without end marker
-            if let Ok(false) = reader.has_end_marker() {
-                worker.log("missing stream end marker".to_string());
-                return Ok(None);
+                if blob.crypt_mode()? == CryptMode::None {
+                    blob.decode(None, Some(&digest))?; // verify digest
+                }
+                if verbose {
+                    task_log!(worker, "Insert chunk: {}", proxmox::tools::digest_to_hex(&digest));
+                }
+                datastore.insert_chunk(&blob, &digest)?;
+            } else if verbose {
+                task_log!(worker, "Found existing chunk: {}", proxmox::tools::digest_to_hex(&digest));
             }
-
-            // else the archive is corrupt
-            Err(err)
+        } else if verbose {
+            task_log!(worker, "Found chunk: {}", proxmox::tools::digest_to_hex(&digest));
         }
+        chunks.push(digest);
     }
+
+    Ok(Some(chunks))
 }
 
 fn restore_snapshot_archive<'a>(
-- 
2.20.1

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

[Dominik Csapak]

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