Re: [pbs-devel] [PATCH proxmox-backup v2] api2/tape/restore: restore_snapshot_archive: only ignore tape errors

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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