[pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Apr 14 10:41:38 CEST 2021
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 at 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
More information about the pbs-devel
mailing list