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

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

> On 04/16/2021 12:28 PM Dominik Csapak <d.csapak@proxmox.com> wrote:
> 
>  
> fixes:
> * manual implementation of an assign operation
> * using clone on a copy type
> * if chain rewritten with match on Ordering
> * put part of complex type in type definition
> * unnecessary return
> * collapsible if chain
> * inconsistent underscore
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/tape/drive/lto/mod.rs         | 30 +++++++++++++++++-------------
>  src/tape/drive/lto/sg_tape.rs     | 18 ++++++++----------
>  src/tape/drive/lto/sg_tape/mam.rs | 12 +++++++-----
>  src/tape/drive/mod.rs             |  4 +++-
>  src/tape/drive/virtual_tape.rs    |  4 ++--
>  5 files changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/src/tape/drive/lto/mod.rs b/src/tape/drive/lto/mod.rs
> index 642c3cc7..ac4d9b93 100644
> --- a/src/tape/drive/lto/mod.rs
> +++ b/src/tape/drive/lto/mod.rs
> @@ -225,18 +225,22 @@ impl LtoTapeHandle {
>  
>          let current_position = self.current_file_number()?;
>  
> -        if current_position == position {
> -            // make sure we are immediated afer the filemark
> -            self.sg_tape.space_filemarks(-1)?;
> -            self.sg_tape.space_filemarks(1)?;
> -        } else if current_position < position {
> -            let diff = position - current_position;
> -            self.sg_tape.space_filemarks(diff.try_into()?)?;
> -        } else {
> -            let diff = current_position - position + 1;
> -            self.sg_tape.space_filemarks(-diff.try_into()?)?;
> -            // move to EOT side of filemark
> -            self.sg_tape.space_filemarks(1)?;
> +        match current_position.cmp(&position) {
> +            std::cmp::Ordering::Equal => {
> +                // make sure we are immediated afer the filemark
> +                self.sg_tape.space_filemarks(-1)?;
> +                self.sg_tape.space_filemarks(1)?;
> +            }
> +            std::cmp::Ordering::Less => {
> +                let diff = position - current_position;
> +                self.sg_tape.space_filemarks(diff.try_into()?)?;
> +            }
> +            std::cmp::Ordering::Greater => {
> +                let diff = current_position - position + 1;
> +                self.sg_tape.space_filemarks(-diff.try_into()?)?;
> +                // move to EOT side of filemark
> +                self.sg_tape.space_filemarks(1)?;
> +            }
>          }
>  
>          Ok(())
> @@ -409,7 +413,7 @@ impl TapeDriver for LtoTapeHandle {
>  
>                          let mut tape_key = [0u8; 32];
>  
> -                        let uuid_bytes: [u8; 16] = uuid.as_bytes().clone();
> +                        let uuid_bytes: [u8; 16] = *uuid.as_bytes();

^ IMO this doesn't even need to be copied?
We only use it as salt for the pkbdf2_hmac call which just needs an immutable byte slice.

>  
>                          openssl::pkcs5::pbkdf2_hmac(
>                              &item.key,
> diff --git a/src/tape/drive/lto/sg_tape.rs b/src/tape/drive/lto/sg_tape.rs
> index 7faa1639..aba55f4d 100644
> --- a/src/tape/drive/lto/sg_tape.rs
> +++ b/src/tape/drive/lto/sg_tape.rs
> @@ -82,7 +82,7 @@ impl DataCompressionModePage {
>          if enable {
>              self.flags2 |= 128;
>          } else {
> -            self.flags2 = self.flags2 & 127;
> +            self.flags2 &= 127;
>          }
>      }
>  
> @@ -310,10 +310,8 @@ impl SgTape {
>          sg_raw.do_command(&cmd)
>              .map_err(|err| format_err!("move to EOD failed - {}", err))?;
>  
> -        if write_missing_eof {
> -            if !self.check_filemark()? {
> -                self.write_filemarks(1, false)?;
> -            }
> +        if write_missing_eof && !self.check_filemark()? {
> +            self.write_filemarks(1, false)?;
>          }
>  
>          Ok(())
> @@ -476,7 +474,7 @@ impl SgTape {
>  
>      /// Read Volume Statistics
>      pub fn volume_statistics(&mut self) -> Result<Lp17VolumeStatistics, Error> {
> -        return read_volume_statistics(&mut self.file);
> +        read_volume_statistics(&mut self.file)
>      }
>  
>      pub fn set_encryption(
> @@ -516,9 +514,9 @@ impl SgTape {
>          //println!("WRITE {:?}", data);
>  
>          match sg_raw.do_out_command(&cmd, data) {
> -            Ok(()) => { return Ok(false) }
> +            Ok(()) => { Ok(false) }

^ Could have also skipped the braces ;-)

>              Err(ScsiError::Sense(SenseInfo { sense_key: 0, asc: 0, ascq: 2 })) => {
> -                return Ok(true); // LEOM
> +                Ok(true) // LEOM
>              }
>              Err(err) => {
>                  proxmox::io_bail!("write failed - {}", err);
> @@ -608,9 +606,9 @@ impl SgTape {
>          }
>  
>          if let Some(buffer_mode) = buffer_mode {
> -            let mut mode = head.flags3 & 0b1_000_1111;
> +            let mut mode = head.flags3 & 0b1000_1111;

^ I really wish those bits were documented or `bitflags!`, because maybe the 1/3/4 grouping is based on meaning :S
But since it's not, I agree with this hunk ;-)

>              if buffer_mode {
> -                mode |= 0b0_001_0000;
> +                mode |= 0b0001_0000;
>              }
>              head.flags3 = mode;
>          }
> diff --git a/src/tape/drive/lto/sg_tape/mam.rs b/src/tape/drive/lto/sg_tape/mam.rs
> index 06b81850..37dd1a8e 100644
> --- a/src/tape/drive/lto/sg_tape/mam.rs
> +++ b/src/tape/drive/lto/sg_tape/mam.rs
> @@ -132,11 +132,13 @@ fn decode_mam_attributes(data: &[u8]) -> Result<Vec<MamAttribute>, Error> {
>      let expected_len = data_len as usize;
>  
>  
> -    if reader.len() < expected_len {
> -        bail!("read_mam_attributes: got unexpected data len ({} != {})", reader.len(), expected_len);
> -    } else if reader.len() > expected_len {
> -        // Note: Quantum hh7 returns the allocation_length instead of real data_len
> -        reader = &data[4..expected_len+4];
> +    match reader.len().cmp(&expected_len) {
> +        std::cmp::Ordering::Less => bail!("read_mam_attributes: got unexpected data len ({} != {})", reader.len(), expected_len),
> +        std::cmp::Ordering::Greater => {
> +            // Note: Quantum hh7 returns the allocation_length instead of real data_len
> +            reader = &data[4..expected_len+4];
> +        }
> +        std::cmp::Ordering::Equal => {}, // expected
>      }
>  
>      let mut list = Vec::new();
> diff --git a/src/tape/drive/mod.rs b/src/tape/drive/mod.rs
> index fd8f503d..1b436351 100644
> --- a/src/tape/drive/mod.rs
> +++ b/src/tape/drive/mod.rs
> @@ -231,6 +231,8 @@ pub trait TapeDriver {
>      }
>  }
>  
> +type DriveHandleAndName = (Box<dyn MediaChange>, String);
> +

I'm less of a fan of this one.
A struct, or a different function name with a clippy-shutup-hint might be nicer.
Also, isn't it enough for clippy if just the trait object is named? I think that one
adds than a mere 2-tuple.

>  /// Get the media changer (MediaChange + name) associated with a tape drive.
>  ///
>  /// Returns Ok(None) if the drive has no associated changer device.
> @@ -241,7 +243,7 @@ pub trait TapeDriver {
>  pub fn media_changer(
>      config: &SectionConfigData,
>      drive: &str,
> -) -> Result<Option<(Box<dyn MediaChange>, String)>, Error> {
> +) -> Result<Option<DriveHandleAndName>, Error> {
>  
>      match config.sections.get(drive) {
>          Some((section_type_name, config)) => {
> diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
> index f7ebc0bb..2187e79c 100644
> --- a/src/tape/drive/virtual_tape.rs
> +++ b/src/tape/drive/virtual_tape.rs
> @@ -215,7 +215,7 @@ impl VirtualTapeHandle {
>              Some(VirtualTapeStatus { ref mut pos, .. }) => {
>  
>                  if count <= *pos {
> -                    *pos = *pos - count;
> +                    *pos -= count;
>                  } else {
>                      bail!("backward_space_count_files failed: move before BOT");
>                  }
> @@ -290,7 +290,7 @@ impl TapeDriver for VirtualTapeHandle {
>                  Ok(Box::new(reader))
>              }
>              None => {
> -                return Err(BlockReadError::Error(proxmox::io_format_err!("drive is empty (no tape loaded).")));
> +                Err(BlockReadError::Error(proxmox::io_format_err!("drive is empty (no tape loaded).")))
>              }
>          }
>      }
> -- 
> 2.20.1