* Re: [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
@ 2021-04-19 9:35 Dietmar Maurer
0 siblings, 0 replies; 6+ messages in thread
From: Dietmar Maurer @ 2021-04-19 9:35 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Wolfgang Bumiller,
Dominik Csapak
> > @@ -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;
This is documented in the SCSI reference. Please do not change this!
> ^ I really wish those bits were documented or `bitflags!`, because maybe the 1/3/4 grouping is based on meaning :S
yes, It has meaning
> But since it's not, I agree with this hunk ;-)
>
> > if buffer_mode {
> > - mode |= 0b0_001_0000;
> > + mode |= 0b0001_0000;
again , please do not change. Bits are grouped by meaning.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
@ 2021-04-19 15:46 Dietmar Maurer
0 siblings, 0 replies; 6+ messages in thread
From: Dietmar Maurer @ 2021-04-19 15:46 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Wolfgang Bumiller,
Thomas Lamprecht, Dominik Csapak
> > But it is though, they are from the SCSI reference
> > "Mode Parameter Header for Mode Select"
> >
> > +--+--+--+--+--+--+--+--+
> > |WP| B.MODE | SPEED |
> > +--+--+--+--+--+--+--+--+
> >
> > (write protect, buffer mode, speed)
> >
> > so this is the wrong solution, the right one would be:
> > * keep as is
> > * introduce constant like BUFFER_MODE_MASK 0b01110000 and use it negated here
>
> ^ So let's do this then.
>
> I do think this particular lint is the least useful one TBH. When working on embedded
> riscv code I was pretty much constantly fighting with it...
>
> And maybe when the base is done we should also go through all the data structures and
> add doc comments referencing the corresponding scsi documentation to make it easier
> to navigate?
Instead, I would search for a real solution, some kind of bitfield:
https://immunant.com/blog/2020/01/bitfields/
Not sure whats best ...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
@ 2021-04-19 12:09 Wolfgang Bumiller
0 siblings, 0 replies; 6+ messages in thread
From: Wolfgang Bumiller @ 2021-04-19 12:09 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion,
Dominik Csapak
> On 04/19/2021 11:40 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>
>
> On 19.04.21 10:38, Wolfgang Bumiller wrote:
> >> 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 ;-)
>
> But it is though, they are from the SCSI reference
> "Mode Parameter Header for Mode Select"
>
> +--+--+--+--+--+--+--+--+
> |WP| B.MODE | SPEED |
> +--+--+--+--+--+--+--+--+
>
> (write protect, buffer mode, speed)
>
> so this is the wrong solution, the right one would be:
> * keep as is
> * introduce constant like BUFFER_MODE_MASK 0b01110000 and use it negated here
^ So let's do this then.
I do think this particular lint is the least useful one TBH. When working on embedded
riscv code I was pretty much constantly fighting with it...
And maybe when the base is done we should also go through all the data structures and
add doc comments referencing the corresponding scsi documentation to make it easier
to navigate?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
2021-04-19 8:38 Wolfgang Bumiller
@ 2021-04-19 9:40 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2021-04-19 9:40 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Wolfgang Bumiller,
Dominik Csapak
On 19.04.21 10:38, Wolfgang Bumiller wrote:
>> 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 ;-)
But it is though, they are from the SCSI reference
"Mode Parameter Header for Mode Select"
+--+--+--+--+--+--+--+--+
|WP| B.MODE | SPEED |
+--+--+--+--+--+--+--+--+
(write protect, buffer mode, speed)
so this is the wrong solution, the right one would be:
* keep as is
* introduce constant like BUFFER_MODE_MASK 0b01110000 and use it negated here
* use bitflags or the like
When reading the actual code and having the SCSI reference open (which is a must
when working on such code anyway) it's actually quite clear what the intention
is.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
@ 2021-04-19 8:38 Wolfgang Bumiller
2021-04-19 9:40 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2021-04-19 8:38 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes
2021-04-16 10:28 [pbs-devel] [PATCH proxmox-backup v2 00/14] various " Dominik Csapak
@ 2021-04-16 10:28 ` Dominik Csapak
0 siblings, 0 replies; 6+ messages in thread
From: Dominik Csapak @ 2021-04-16 10:28 UTC (permalink / raw)
To: pbs-devel
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();
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) }
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;
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);
+
/// 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-19 15:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 9:35 [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: clippy fixes Dietmar Maurer
-- strict thread matches above, loose matches on Subject: below --
2021-04-19 15:46 Dietmar Maurer
2021-04-19 12:09 Wolfgang Bumiller
2021-04-19 8:38 Wolfgang Bumiller
2021-04-19 9:40 ` Thomas Lamprecht
2021-04-16 10:28 [pbs-devel] [PATCH proxmox-backup v2 00/14] various " Dominik Csapak
2021-04-16 10:28 ` [pbs-devel] [PATCH proxmox-backup v2 03/14] tape/drive: " Dominik Csapak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox