Re: [pbs-devel] [PATCH proxmox-backup v2 09/14] tape/*: clippy fixes

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

> On 04/16/2021 12:29 PM Dominik Csapak <d.csapak@proxmox.com> wrote:
> 
>  
> fixes:
> * absurd extreme comparisons
> * or_fun_call
> * unnecessary return
> * collapsible if chain
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/tape/file_formats/blocked_reader.rs      |  4 ++--
>  src/tape/file_formats/chunk_archive.rs       | 12 +++++-------
>  src/tape/file_formats/multi_volume_writer.rs |  2 +-
>  src/tape/inventory.rs                        |  2 +-
>  4 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/src/tape/file_formats/blocked_reader.rs b/src/tape/file_formats/blocked_reader.rs
> index ce2fc9aa..bfdbc2e7 100644
> --- a/src/tape/file_formats/blocked_reader.rs
> +++ b/src/tape/file_formats/blocked_reader.rs
> @@ -118,13 +118,13 @@ impl <R: BlockRead> BlockedReader<R> {
>                  proxmox::io_bail!("detected tape block after block-stream end marker");
>              }
>              Err(BlockReadError::EndOfFile) => {
> -                return Ok(());
> +                Ok(())
>              }
>              Err(BlockReadError::EndOfStream) => {
>                  proxmox::io_bail!("got unexpected end of tape");
>              }
>              Err(BlockReadError::Error(err)) => {
> -                return Err(err);
> +                Err(err)
>              }
>          }
>      }
> diff --git a/src/tape/file_formats/chunk_archive.rs b/src/tape/file_formats/chunk_archive.rs
> index f6b07806..ab4234dd 100644
> --- a/src/tape/file_formats/chunk_archive.rs
> +++ b/src/tape/file_formats/chunk_archive.rs
> @@ -120,13 +120,11 @@ impl <'a> ChunkArchiveWriter<'a> {
>              } else {
>                  self.write_all(&blob_data[start..end])?
>              };
> -            if leom {
> -                if self.close_on_leom {
> -                    let mut writer = self.writer.take().unwrap();
> -                    writer.finish(false)?;
> -                    self.bytes_written = writer.bytes_written();
> -                    return Ok(chunk_is_complete);
> -                }
> +            if leom && self.close_on_leom {
> +                let mut writer = self.writer.take().unwrap();
> +                writer.finish(false)?;
> +                self.bytes_written = writer.bytes_written();
> +                return Ok(chunk_is_complete);
>              }
>              start = end;
>          }
> diff --git a/src/tape/file_formats/multi_volume_writer.rs b/src/tape/file_formats/multi_volume_writer.rs
> index d58285e9..8e0e1d94 100644
> --- a/src/tape/file_formats/multi_volume_writer.rs
> +++ b/src/tape/file_formats/multi_volume_writer.rs
> @@ -72,7 +72,7 @@ impl <'a> TapeWrite for MultiVolumeWriter<'a> {
>          }
>  
>          if self.writer.is_none() {
> -            if self.header.part_number >= 255 {
> +            if self.header.part_number == 255 {

So this caught my attention.

This method increments this to at most 255, which makes sense, given that this is an u8.
(But the incrementation & check could be closer together, be a separate method, or use .checked_add()

But what I'm worried about is the reader side where we have:

    let expect_part_number = self.header.part_number + 1;

This should probably also sanity-check this possible overflow in case of bad/corrupted/fabricated data, no?

So if `255` is a valid number

>                  proxmox::io_bail!("multi-volume writer: too many parts");
>              }
>              self.writer = Some(
> diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
> index f9654538..15a51e5e 100644
> --- a/src/tape/inventory.rs
> +++ b/src/tape/inventory.rs
> @@ -792,7 +792,7 @@ pub fn lock_media_set(
>      path.push(format!(".media-set-{}", media_set_uuid));
>      path.set_extension("lck");
>  
> -    let timeout = timeout.unwrap_or(Duration::new(10, 0));
> +    let timeout = timeout.unwrap_or_else(|| Duration::new(10, 0));

Oof, why is `Duration::new` not a `const fn` O.o

>      let file = open_file_locked(&path, timeout, true)?;
>      if cfg!(test) {
>          // We cannot use chown inside test environment (no permissions)
> -- 
> 2.20.1