From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 1A332729D7
for <pbs-devel@lists.proxmox.com>; Tue, 13 Apr 2021 14:05:41 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 0FA482A422
for <pbs-devel@lists.proxmox.com>; Tue, 13 Apr 2021 14:05:41 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[212.186.127.180])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 384732A419
for <pbs-devel@lists.proxmox.com>; Tue, 13 Apr 2021 14:05:39 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D444B45A6A
for <pbs-devel@lists.proxmox.com>; Tue, 13 Apr 2021 14:05:38 +0200 (CEST)
From: Dominik Csapak <d.csapak@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Tue, 13 Apr 2021 14:05:37 +0200
Message-Id: <20210413120537.13220-1-d.csapak@proxmox.com>
X-Mailer: git-send-email 2.20.1
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.164 Adjusted score from AWL reputation of From: address
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/,
medium trust
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [restore.rs]
Subject: [pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore:
restore_snapshot_archive: only ignore tape errors
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Tue, 13 Apr 2021 12:05:41 -0000
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