From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; Tue, 13 Apr 2021 14:05:38 +0200 (CEST) From: Dominik Csapak 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 --- 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, @@ -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( worker: &WorkerTask, decoder: &mut pxar::decoder::sync::Decoder, 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( 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( 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( 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( 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