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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E598F7294D for ; Tue, 13 Apr 2021 12:59:04 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D24EA29D19 for ; Tue, 13 Apr 2021 12:59:04 +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 E2CD429D07 for ; Tue, 13 Apr 2021 12:59:00 +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 B2B5E441CB for ; Tue, 13 Apr 2021 12:59:00 +0200 (CEST) From: Dominik Csapak To: pbs-devel@lists.proxmox.com Date: Tue, 13 Apr 2021 12:58:59 +0200 Message-Id: <20210413105859.6760-2-d.csapak@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20210413105859.6760-1-d.csapak@proxmox.com> References: <20210413105859.6760-1-d.csapak@proxmox.com> 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 2/2] 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 10:59:04 -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', which maps anyhow Errors as type 'Tape' and io Errors as type 'Disk' Signed-off-by: Dominik Csapak --- 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, @@ -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( 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 = 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( 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( 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( 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