[pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
Wolfgang Bumiller
w.bumiller at proxmox.com
Mon Apr 19 10:38:51 CEST 2021
> On 04/16/2021 12:28 PM Dominik Csapak <d.csapak at 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 at 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
More information about the pbs-devel
mailing list