* Re: [pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors
@ 2021-04-14 8:51 Dietmar Maurer
0 siblings, 0 replies; 3+ messages in thread
From: Dietmar Maurer @ 2021-04-14 8:51 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Wolfgang Bumiller,
Dominik Csapak
> 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<T: ToString>(err: T) -> Self {
> RestoreError::Other(err.to_string())
> }
>
> fn disk<T: ToString>(err: T) -> Self {
> RestoreError::Disk(err.to_string())
> }
>
> fn tape<T: Into<Error>>(err: T) -> Self {
> RestoreError::Tape(err.into())
> }
> }
I like that idea, but I don't think it is a good idea to convert Errors to strings.
So I would use:
#[derive(thiserror::Error, Debug)]
pub enum RestoreError {
#[error("tape error: {0}")]
Tape(Error),
#[error("disk error: {0}")]
Disk(Error),
#[error("{0}")]
Other(Error),
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors
2021-04-13 12:05 Dominik Csapak
@ 2021-04-14 8:41 ` Wolfgang Bumiller
0 siblings, 0 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2021-04-14 8:41 UTC (permalink / raw)
To: Dominik Csapak; +Cc: pbs-devel
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 <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),
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<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(),
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<R: pxar::decoder::SeqRead>(
> 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<T: ToString>(err: T) -> Self {
RestoreError::Other(err.to_string())
}
fn disk<T: ToString>(err: T) -> Self {
RestoreError::Disk(err.to_string())
}
fn tape<T: Into<Error>>(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<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()))?;
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<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()))?;
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<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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors
@ 2021-04-13 12:05 Dominik Csapak
2021-04-14 8:41 ` Wolfgang Bumiller
0 siblings, 1 reply; 3+ messages in thread
From: Dominik Csapak @ 2021-04-13 12:05 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-14 8:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 8:51 [pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors Dietmar Maurer
-- strict thread matches above, loose matches on Subject: below --
2021-04-13 12:05 Dominik Csapak
2021-04-14 8:41 ` Wolfgang Bumiller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal