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 BC69F72DA9 for ; Wed, 14 Apr 2021 10:42:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A8BACBAE9 for ; Wed, 14 Apr 2021 10:41:42 +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 E5852BADC for ; Wed, 14 Apr 2021 10:41:40 +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 AA2FB420E5 for ; Wed, 14 Apr 2021 10:41:40 +0200 (CEST) Date: Wed, 14 Apr 2021 10:41:38 +0200 From: Wolfgang Bumiller To: Dominik Csapak Cc: pbs-devel@lists.proxmox.com Message-ID: <20210414084138.x3q7hopaznjwdjbm@wobu-vie.proxmox.com> References: <20210413120537.13220-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210413120537.13220-1-d.csapak@proxmox.com> User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 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: Re: [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: Wed, 14 Apr 2021 08:42:12 -0000 code lgtm, though I'm deep enough in the tape code to apply this, so I'll leave applying to Dietmar. Some quality-of-life hints for future changes of the sort though inline: On Tue, Apr 13, 2021 at 02:05:37PM +0200, Dominik Csapak wrote: > 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), As a preview for upcoming rust changes (this one should be in 1.53), or-patterns will allow writing this without repeating the `Err()` in the future: Err(RestoreError::Other(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(), The `.into()` can be dropped here. > + )); > + } > > 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)))?; Since the format string is just `"{}"` you could just use `err.to_string()` (which you do in other cases later anyway) Now, while this is not happening a lot for the `Other` case, it does happen quite a few times for the `Disk` case, so you could also add a few little helpers for some "quality of life" improvements: impl RestoreError { fn other(err: T) -> Self { RestoreError::Other(err.to_string()) } fn disk(err: T) -> Self { RestoreError::Disk(err.to_string()) } fn tape>(err: T) -> Self { RestoreError::Tape(err.into()) } } Allowing you to write `.map_err(RestoreError::disk)` for anything that can be stringified (so literally all error types ;-)), and `.map_err(RestoreError::tape)` for anything convertible to an `anyhow::Error` (so again, all error types ;-)). (Note the lowercase names in the method names in my comments below) > > let entry = match decoder.next() { > None => break, > - Some(entry) => entry?, > + Some(entry) => entry.map_err(|err| RestoreError::Tape(err.into()))?, Then this would be: Some(entry) => entry.map_err(RestoreError::tape)?, > }; > + > 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 > + ))); > } I think this is the point I would have introduced `tape_format_err!()` and `tape_bail!()` macros, they're not as long to write as you might think ;-) (Just look at the `io_*` variants we have in proxmox::sys::macros) > > 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()))?; With the helpers: .map_err(RestoreError::disk)?; > > - 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()))?; With the helpers: .map_err(RestoreError::tape)?; > + > + 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()))?; Another: .map_err(RestoreError::disk)?; > 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()))?; and here > 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()))?; and here > + let (csum, size) = compute_file_csum(&mut tmpfile) > + .map_err(|err| RestoreError::Disk(err.to_string()))?; and here > 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