From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 123BF74648 for ; Mon, 19 Apr 2021 10:39:47 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0039F12233 for ; Mon, 19 Apr 2021 10:39:17 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 1456612224 for ; Mon, 19 Apr 2021 10:39:13 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id CDE4C44F84 for ; Mon, 19 Apr 2021 10:39:12 +0200 (CEST) Date: Mon, 19 Apr 2021 10:38:51 +0200 (CEST) From: Wolfgang Bumiller To: Proxmox Backup Server development discussion , Dominik Csapak Message-ID: <1839417922.3844.1618821531697@webmail.proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Priority: 3 Importance: Normal X-Mailer: Open-Xchange Mailer v7.10.5-Rev5 X-Originating-Client: open-xchange-appsuite X-SPAM-LEVEL: Spam detection results: 0 AWL -0.118 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_2 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_4 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Apr 2021 08:39:47 -0000 > On 04/16/2021 12:28 PM Dominik Csapak 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 > --- > 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 { > - 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, 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, 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, String)>, Error> { > +) -> Result, 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