[pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors
Dominik Csapak
d.csapak at proxmox.com
Tue Apr 13 14:05:37 CEST 2021
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 at 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
More information about the pbs-devel
mailing list