[pbs-devel] [PATCH proxmox-backup v2 00/14] various clippy fixes

[pbs-devel] [PATCH proxmox-backup v2 00/14] various clippy fixes

[Dominik Csapak]

rebased on master, fixed new errors/warnings that popped up

Dominik Csapak (14):
  api2/tape: clippy fixes
  tape/changer: clippy fixes
  tape/drive: clippy fixes
  tape/media_*: clippy fixes
  tape/pool_writer: clippy fixes
  backup: clippy fixes
  pxar: clippy fix `or_fun_call`
  bin: clippy fixes
  tape/*: clippy fixes
  tools: clippy fixes
  config/tape_encryption_keys: clippy fixes
  server/worker_task: clippy fix
  bin/proxmox-file-restore: clippy fixes
  bin/proxmox-restore-daemon: clippy fixes

 src/api2/config/tape_backup_job.rs            |  1 +
 src/api2/tape/backup.rs                       |  2 +-
 src/api2/tape/media.rs                        | 42 +++++++++----------
 src/api2/tape/restore.rs                      | 12 +++---
 src/backup/key_derivation.rs                  |  6 +--
 src/backup/manifest.rs                        |  1 +
 src/bin/docgen.rs                             |  2 +-
 src/bin/pmtx.rs                               |  2 +-
 src/bin/proxmox-file-restore.rs               |  4 +-
 .../proxmox_file_restore/block_driver_qemu.rs |  2 +-
 src/bin/proxmox_file_restore/qemu_helper.rs   |  4 +-
 src/bin/proxmox_restore_daemon/auth.rs        |  8 ++--
 src/config/tape_encryption_keys.rs            |  8 ++--
 src/pxar/extract.rs                           |  4 +-
 src/server/worker_task.rs                     |  2 +-
 src/tape/changer/online_status_map.rs         |  4 +-
 src/tape/changer/sg_pt_changer.rs             |  2 +-
 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 +-
 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 +-
 src/tape/media_catalog.rs                     | 10 +++--
 src/tape/media_pool.rs                        | 16 +++----
 src/tape/media_set.rs                         |  4 ++
 src/tape/pool_writer/catalog_set.rs           |  1 +
 src/tape/pool_writer/mod.rs                   |  4 +-
 src/tape/pool_writer/new_chunks_iterator.rs   |  4 +-
 src/tools/cpio.rs                             |  1 +
 src/tools/fs.rs                               |  3 ++
 src/tools/sgutils2.rs                         | 18 ++++----
 35 files changed, 135 insertions(+), 120 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 01/14] api2/tape: clippy fixes

[Dominik Csapak]

fixes:
* too_many_arguments
* `if let Err(_) = ` => `.is_err()`
* combine if branches
* remove unnecessary lifetime
* remove unnecessary return
* `.len() == 0` => `.is_empty()`
* `find().is_some()` => `.any()`

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/config/tape_backup_job.rs |  1 +
 src/api2/tape/backup.rs            |  2 +-
 src/api2/tape/media.rs             | 42 ++++++++++++++----------------
 src/api2/tape/restore.rs           | 12 ++++-----
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
index caea4e18..70d1f6af 100644
--- a/src/api2/config/tape_backup_job.rs
+++ b/src/api2/config/tape_backup_job.rs
@@ -219,6 +219,7 @@ pub enum DeletableProperty {
     },
 )]
 /// Update the tape backup job
+#[allow(clippy::too_many_arguments)]
 pub fn update_tape_backup_job(
     id: String,
     store: Option<String>,
diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index bce0fbcc..18153048 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -607,7 +607,7 @@ pub fn backup_snapshot(
         }
     }
 
-    if let Err(_) = reader_thread.join() {
+    if reader_thread.join().is_err() {
         bail!("chunk reader thread failed");
     }
 
diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs
index 811fcb7e..b1497fb3 100644
--- a/src/api2/tape/media.rs
+++ b/src/api2/tape/media.rs
@@ -173,32 +173,30 @@ pub async fn list_media(
     let inventory = Inventory::load(status_path)?;
 
     let privs = user_info.lookup_privs(&auth_id, &["tape", "pool"]);
-    if (privs & PRIV_TAPE_AUDIT) != 0  {
-        if pool.is_none() {
+    if (privs & PRIV_TAPE_AUDIT) != 0 && pool.is_none() {
 
-            for media_id in inventory.list_unassigned_media() {
+        for media_id in inventory.list_unassigned_media() {
 
-                let (mut status, location) = inventory.status_and_location(&media_id.label.uuid);
+            let (mut status, location) = inventory.status_and_location(&media_id.label.uuid);
 
-                if status == MediaStatus::Unknown {
-                    status = MediaStatus::Writable;
-                }
-
-                list.push(MediaListEntry {
-                    uuid: media_id.label.uuid.clone(),
-                    ctime: media_id.label.ctime,
-                    label_text: media_id.label.label_text.to_string(),
-                    location,
-                    status,
-                    catalog: true, // empty, so we do not need a catalog
-                    expired: false,
-                    media_set_uuid: None,
-                    media_set_name: None,
-                    media_set_ctime: None,
-                    seq_nr: None,
-                    pool: None,
-                });
+            if status == MediaStatus::Unknown {
+                status = MediaStatus::Writable;
             }
+
+            list.push(MediaListEntry {
+                uuid: media_id.label.uuid.clone(),
+                ctime: media_id.label.ctime,
+                label_text: media_id.label.label_text.to_string(),
+                location,
+                status,
+                catalog: true, // empty, so we do not need a catalog
+                expired: false,
+                media_set_uuid: None,
+                media_set_name: None,
+                media_set_ctime: None,
+                seq_nr: None,
+                pool: None,
+            });
         }
     }
 
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 5cadd6dd..45a035d0 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -137,7 +137,7 @@ impl TryFrom<String> for DataStoreMap {
 }
 
 impl DataStoreMap {
-    fn used_datastores<'a>(&self) -> HashSet<&str> {
+    fn used_datastores(&self) -> HashSet<&str> {
         let mut set = HashSet::new();
         for store in self.map.values() {
             set.insert(store.name());
@@ -158,7 +158,7 @@ impl DataStoreMap {
             return Some(&store);
         }
 
-        return None;
+        None
     }
 }
 
@@ -212,7 +212,7 @@ pub fn restore(
     let store_map = DataStoreMap::try_from(store)
         .map_err(|err| format_err!("cannot parse store mapping: {}", err))?;
     let used_datastores = store_map.used_datastores();
-    if used_datastores.len() == 0 {
+    if used_datastores.is_empty() {
         bail!("no datastores given");
     }
 
@@ -369,6 +369,7 @@ pub fn restore(
 }
 
 /// Request and restore complete media without using existing catalog (create catalog instead)
+#[allow(clippy::too_many_arguments)]
 pub fn request_and_restore_media(
     worker: &WorkerTask,
     media_id: &MediaId,
@@ -878,13 +879,12 @@ pub fn fast_catalog_restore(
                 let wanted = media_set
                     .media_list()
                     .iter()
-                    .find(|e| {
+                    .any(|e| {
                         match e {
                             None => false,
                             Some(uuid) => uuid == catalog_uuid,
                         }
-                    })
-                    .is_some();
+                    });
 
                 if !wanted {
                     task_log!(worker, "skip catalog because media '{}' not inventarized", catalog_uuid);
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 02/14] tape/changer: clippy fixes

[Dominik Csapak]

fixes:
* unnecessary referencing
* `.map(|v| *v)` => `.copied()`

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/changer/online_status_map.rs | 4 ++--
 src/tape/changer/sg_pt_changer.rs     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/tape/changer/online_status_map.rs b/src/tape/changer/online_status_map.rs
index e30cd17b..324d1fec 100644
--- a/src/tape/changer/online_status_map.rs
+++ b/src/tape/changer/online_status_map.rs
@@ -137,7 +137,7 @@ pub fn update_online_status(state_path: &Path, changer: Option<&str>) -> Result<
 
     for mut changer_config in changers {
         if let Some(changer) = changer {
-            if changer != &changer_config.name {
+            if changer != changer_config.name {
                 continue;
             }
             found_changer = true;
@@ -157,7 +157,7 @@ pub fn update_online_status(state_path: &Path, changer: Option<&str>) -> Result<
     let vtapes: Vec<VirtualTapeDrive> = config.convert_to_typed_array("virtual")?;
     for mut vtape in vtapes {
         if let Some(changer) = changer {
-            if changer != &vtape.name {
+            if changer != vtape.name {
                 continue;
             }
             found_changer = true;
diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index 785fc9ce..b559653c 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -392,7 +392,7 @@ pub fn read_element_status<F: AsRawFd>(file: &mut F) -> Result<MtxStatus, Error>
     for drive in status.drives.iter_mut() {
         if let Some(source_address) = drive.loaded_slot {
             let source_address = source_address as u16;
-            drive.loaded_slot = slot_map.get(&source_address).map(|v| *v);
+            drive.loaded_slot = slot_map.get(&source_address).copied();
         }
     }
 
-- 
2.20.1

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

[Dominik Csapak]

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

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

[Dominik Csapak]

fixes:
* impl (or derive) Default if struct has 'new()'
* use `or_insert_with`
* combine if branches

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/media_catalog.rs | 10 ++++++----
 src/tape/media_pool.rs    | 16 ++++++----------
 src/tape/media_set.rs     |  4 ++++
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
index aff91c43..ac11346a 100644
--- a/src/tape/media_catalog.rs
+++ b/src/tape/media_catalog.rs
@@ -30,6 +30,7 @@ use crate::{
     },
 };
 
+#[derive(Default)]
 pub struct DatastoreContent {
     pub snapshot_index: HashMap<String, u64>, // snapshot => file_nr
     pub chunk_index: HashMap<[u8;32], u64>, // chunk => file_nr
@@ -580,7 +581,7 @@ impl MediaCatalog {
         unsafe { self.pending.write_le_value(entry)?; }
         self.pending.extend(store.as_bytes());
 
-        self.content.entry(store.to_string()).or_insert(DatastoreContent::new());
+        self.content.entry(store.to_string()).or_insert_with(DatastoreContent::new);
 
         self.current_archive = Some((uuid, file_number, store.to_string()));
 
@@ -688,7 +689,7 @@ impl MediaCatalog {
         self.pending.extend(snapshot.as_bytes());
 
         let content = self.content.entry(store.to_string())
-            .or_insert(DatastoreContent::new());
+            .or_insert_with(DatastoreContent::new);
 
         content.snapshot_index.insert(snapshot.to_string(), file_number);
 
@@ -809,7 +810,7 @@ impl MediaCatalog {
                     self.check_start_chunk_archive(file_number)?;
 
                     self.content.entry(store.to_string())
-                        .or_insert(DatastoreContent::new());
+                        .or_insert_with(DatastoreContent::new);
 
                     self.current_archive = Some((uuid, file_number, store.to_string()));
                }
@@ -843,7 +844,7 @@ impl MediaCatalog {
                     self.check_register_snapshot(file_number, snapshot)?;
 
                     let content = self.content.entry(store.to_string())
-                        .or_insert(DatastoreContent::new());
+                        .or_insert_with(DatastoreContent::new);
 
                     content.snapshot_index.insert(snapshot.to_string(), file_number);
 
@@ -884,6 +885,7 @@ impl MediaCatalog {
 /// Media set catalog
 ///
 /// Catalog for multiple media.
+#[derive(Default)]
 pub struct MediaSetCatalog  {
     catalog_list: HashMap<Uuid, MediaCatalog>,
 }
diff --git a/src/tape/media_pool.rs b/src/tape/media_pool.rs
index 039b46fc..51b537c4 100644
--- a/src/tape/media_pool.rs
+++ b/src/tape/media_pool.rs
@@ -348,13 +348,11 @@ impl MediaPool {
             MediaLocation::Online(name) => {
                 if self.force_media_availability {
                     true
+                } else if let Some(ref changer_name) = self.changer_name {
+                    name == changer_name
                 } else {
-                    if let Some(ref changer_name) = self.changer_name {
-                        name == changer_name
-                    } else {
-                        // a standalone drive cannot use media currently inside a library
-                        false
-                    }
+                    // a standalone drive cannot use media currently inside a library
+                    false
                 }
             }
             MediaLocation::Offline => {
@@ -606,10 +604,8 @@ impl MediaPool {
                     let media_location = media.location();
                     if self.location_is_available(media_location) {
                         last_is_writable = true;
-                    } else {
-                        if let MediaLocation::Vault(vault) = media_location {
-                            bail!("writable media offsite in vault '{}'", vault);
-                        }
+                    } else if let MediaLocation::Vault(vault) = media_location {
+                        bail!("writable media offsite in vault '{}'", vault);
                     }
                 },
                 _ => bail!("unable to use media set - wrong media status {:?}", media.status()),
diff --git a/src/tape/media_set.rs b/src/tape/media_set.rs
index 5568e7f6..858a9e77 100644
--- a/src/tape/media_set.rs
+++ b/src/tape/media_set.rs
@@ -74,3 +74,7 @@ impl MediaSet {
         }
     }
 }
+
+impl Default for MediaSet {
+    fn default() -> Self { Self::new() }
+}
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 05/14] tape/pool_writer: clippy fixes

[Dominik Csapak]

fixes:
* impl (or derive) Default for structs with `new()`
* put complex type in type definition

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/pool_writer/catalog_set.rs         | 1 +
 src/tape/pool_writer/mod.rs                 | 4 +++-
 src/tape/pool_writer/new_chunks_iterator.rs | 4 +++-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/tape/pool_writer/catalog_set.rs b/src/tape/pool_writer/catalog_set.rs
index fbca3e97..b3630915 100644
--- a/src/tape/pool_writer/catalog_set.rs
+++ b/src/tape/pool_writer/catalog_set.rs
@@ -12,6 +12,7 @@ use crate::{
 /// Helper to build and query sets of catalogs
 ///
 /// Similar to MediaSetCatalog, but allows to modify the last catalog.
+#[derive(Default)]
 pub struct CatalogSet {
     // read only part
     pub media_set_catalog: MediaSetCatalog,
diff --git a/src/tape/pool_writer/mod.rs b/src/tape/pool_writer/mod.rs
index 99fdb48c..36c4dea0 100644
--- a/src/tape/pool_writer/mod.rs
+++ b/src/tape/pool_writer/mod.rs
@@ -531,6 +531,8 @@ impl PoolWriter {
     }
 }
 
+type WriteChunkArchiveResult = (Vec<[u8;32]>, Uuid, bool, usize);
+
 /// write up to <max_size> of chunks
 fn write_chunk_archive<'a>(
     _worker: &WorkerTask,
@@ -538,7 +540,7 @@ fn write_chunk_archive<'a>(
     chunk_iter: &mut std::iter::Peekable<NewChunksIterator>,
     store: &str,
     max_size: usize,
-) -> Result<(Vec<[u8;32]>, Uuid, bool, usize), Error> {
+) -> Result<WriteChunkArchiveResult, Error> {
 
     let (mut writer, content_uuid) = ChunkArchiveWriter::new(writer, store, true)?;
 
diff --git a/src/tape/pool_writer/new_chunks_iterator.rs b/src/tape/pool_writer/new_chunks_iterator.rs
index 56491356..55ea407d 100644
--- a/src/tape/pool_writer/new_chunks_iterator.rs
+++ b/src/tape/pool_writer/new_chunks_iterator.rs
@@ -15,12 +15,14 @@ use crate::{
     },
 };
 
+type ChunkReceiver = std::sync::mpsc::Receiver<Result<Option<([u8; 32], DataBlob)>, Error>>;
+
 /// Chunk iterator which use a separate thread to read chunks
 ///
 /// The iterator skips duplicate chunks and chunks already in the
 /// catalog.
 pub struct NewChunksIterator {
-    rx: std::sync::mpsc::Receiver<Result<Option<([u8; 32], DataBlob)>, Error>>,
+    rx: ChunkReceiver,
 }
 
 impl NewChunksIterator {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 06/14] backup: clippy fixes

[Dominik Csapak]

fixes:
* clone on copy type
* annotate 'wrong_self_convention'

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/backup/key_derivation.rs | 6 +++---
 src/backup/manifest.rs       | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index 5b46a70c..ece04c4f 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -119,7 +119,7 @@ impl KeyConfig  {
     /// Creates a new, unencrypted key.
     pub fn without_password(raw_key: [u8; 32]) -> Result<Self, Error> {
         // always compute fingerprint
-        let crypt_config = CryptConfig::new(raw_key.clone())?;
+        let crypt_config = CryptConfig::new(raw_key)?;
         let fingerprint = Some(crypt_config.fingerprint());
 
         let created = proxmox::tools::time::epoch_i64();
@@ -186,7 +186,7 @@ impl KeyConfig  {
         let created = proxmox::tools::time::epoch_i64();
 
         // always compute fingerprint
-        let crypt_config = CryptConfig::new(raw_key.clone())?;
+        let crypt_config = CryptConfig::new(*raw_key)?;
         let fingerprint = Some(crypt_config.fingerprint());
 
         Ok(Self {
@@ -257,7 +257,7 @@ impl KeyConfig  {
         let mut result = [0u8; 32];
         result.copy_from_slice(&key);
 
-        let crypt_config = CryptConfig::new(result.clone())?;
+        let crypt_config = CryptConfig::new(result)?;
         let fingerprint = crypt_config.fingerprint();
         if let Some(ref stored_fingerprint) = self.fingerprint {
             if &fingerprint != stored_fingerprint {
diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs
index 47f9cadc..f3a149bd 100644
--- a/src/backup/manifest.rs
+++ b/src/backup/manifest.rs
@@ -148,6 +148,7 @@ impl BackupManifest {
     }
 
     // Generate canonical json
+    #[allow(clippy::wrong_self_convention)]
     fn to_canonical_json(value: &Value) -> Result<Vec<u8>, Error> {
         crate::tools::json::to_canonical_json(value)
     }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 07/14] pxar: clippy fix `or_fun_call`

[Dominik Csapak]

replace ok_or with ok_or_else to do lazy evaluation

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/pxar/extract.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pxar/extract.rs b/src/pxar/extract.rs
index 16b2b499..b52bc503 100644
--- a/src/pxar/extract.rs
+++ b/src/pxar/extract.rs
@@ -517,7 +517,7 @@ where
     let root = decoder.open_root().await?;
     let file = root
         .lookup(&path).await?
-        .ok_or(format_err!("error opening '{:?}'", path.as_ref()))?;
+        .ok_or_else(|| format_err!("error opening '{:?}'", path.as_ref()))?;
 
     let mut prefix = PathBuf::new();
     let mut components = file.entry().path().components();
@@ -668,7 +668,7 @@ where
     let file = root
         .lookup(&path)
         .await?
-        .ok_or(format_err!("error opening '{:?}'", path.as_ref()))?;
+        .ok_or_else(|| format_err!("error opening '{:?}'", path.as_ref()))?;
 
     recurse_files_extractor(&mut extractor, file, verbose).await
 }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 08/14] bin: clippy fixes

[Dominik Csapak]

fixes:
* `len() < 1` => `is_empty()`
* redundant closure
* unnecessary return

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/bin/docgen.rs | 2 +-
 src/bin/pmtx.rs   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/docgen.rs b/src/bin/docgen.rs
index 9df9f33b..1f9657ea 100644
--- a/src/bin/docgen.rs
+++ b/src/bin/docgen.rs
@@ -46,7 +46,7 @@ fn main() -> Result<(), Error> {
 
     let (_prefix, args) = get_args();
 
-    if args.len() < 1 {
+    if args.is_empty() {
         bail!("missing arguments");
     }
 
diff --git a/src/bin/pmtx.rs b/src/bin/pmtx.rs
index 88074002..4a59d624 100644
--- a/src/bin/pmtx.rs
+++ b/src/bin/pmtx.rs
@@ -262,7 +262,7 @@ fn unload(
 
         if let Some(to_slot) = status.find_free_slot(false) {
             sg_pt_changer::unload(&mut file, to_slot, drivenum)?;
-            return Ok(());
+            Ok(())
         } else {
             bail!("Drive '{}' unload failure - no free slot", drivenum);
         }
-- 
2.20.1

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

[Dominik Csapak]

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

[pbs-devel] [PATCH proxmox-backup v2 10/14] tools: clippy fixes

[Dominik Csapak]

fixes:
* needless static lifetimes
* unnecessary returns
* `len() == 0` => `is_empty()`
* too many arguments

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tools/cpio.rs     |  1 +
 src/tools/fs.rs       |  3 +++
 src/tools/sgutils2.rs | 18 +++++++++---------
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/tools/cpio.rs b/src/tools/cpio.rs
index 8800e3ad..48f1470f 100644
--- a/src/tools/cpio.rs
+++ b/src/tools/cpio.rs
@@ -8,6 +8,7 @@ use std::ffi::{CString, CStr};
 use tokio::io::{copy, AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
 
 /// Write a cpio file entry to an AsyncWrite.
+#[allow(clippy::too_many_arguments)]
 pub async fn append_file<W: AsyncWrite + Unpin, R: AsyncRead + Unpin>(
     mut target: W,
     content: R,
diff --git a/src/tools/fs.rs b/src/tools/fs.rs
index 72a530d8..0397113b 100644
--- a/src/tools/fs.rs
+++ b/src/tools/fs.rs
@@ -75,6 +75,9 @@ impl ReadDirEntry {
         self.parent_fd
     }
 
+    /// # Safety
+    ///
+    /// same safety concerns as `std::str::from_utf8_unchecked`
     pub unsafe fn file_name_utf8_unchecked(&self) -> &str {
         std::str::from_utf8_unchecked(self.file_name().to_bytes())
     }
diff --git a/src/tools/sgutils2.rs b/src/tools/sgutils2.rs
index b45d7468..b8600e1b 100644
--- a/src/tools/sgutils2.rs
+++ b/src/tools/sgutils2.rs
@@ -91,7 +91,7 @@ impl SgPt {
 /// Peripheral device type text (see `inquiry` command)
 ///
 /// see [https://en.wikipedia.org/wiki/SCSI_Peripheral_Device_Type]
-pub const PERIPHERAL_DEVICE_TYPE_TEXT: [&'static str; 32] = [
+pub const PERIPHERAL_DEVICE_TYPE_TEXT: [&str; 32] = [
     "Disk Drive",
     "Tape Drive",
     "Printer",
@@ -142,7 +142,7 @@ pub const SENSE_KEY_VOLUME_OVERFLOW: u8 = 0x0d;
 pub const SENSE_KEY_MISCOMPARE: u8      = 0x0e;
 
 /// Sense Key Descriptions
-pub const SENSE_KEY_DESCRIPTIONS: [&'static str; 16] = [
+pub const SENSE_KEY_DESCRIPTIONS: [&str; 16] = [
     "No Sense",
     "Recovered Error",
     "Not Ready",
@@ -464,13 +464,13 @@ impl <'a, F: AsRawFd> SgRaw<'a, F> {
 
         let res_cat = unsafe { get_scsi_pt_result_category(ptvp.as_ptr()) };
         match res_cat {
-            SCSI_PT_RESULT_GOOD => return Ok(()),
+            SCSI_PT_RESULT_GOOD => Ok(()),
             SCSI_PT_RESULT_STATUS => {
                 let status = unsafe { get_scsi_pt_status_response(ptvp.as_ptr()) };
                 if status != 0 {
                     return Err(format_err!("unknown scsi error - status response {}", status).into());
                 }
-                return Ok(());
+                Ok(())
             }
             SCSI_PT_RESULT_SENSE => {
                 if sense_len == 0 {
@@ -506,15 +506,15 @@ impl <'a, F: AsRawFd> SgRaw<'a, F> {
                     }
                 };
 
-                return Err(ScsiError::Sense(sense));
+                Err(ScsiError::Sense(sense))
             }
-            SCSI_PT_RESULT_TRANSPORT_ERR => return Err(format_err!("scsi command failed: transport error").into()),
+            SCSI_PT_RESULT_TRANSPORT_ERR => Err(format_err!("scsi command failed: transport error").into()),
             SCSI_PT_RESULT_OS_ERR => {
                 let errno = unsafe { get_scsi_pt_os_err(ptvp.as_ptr()) };
                 let err = nix::Error::from_errno(nix::errno::Errno::from_i32(errno));
-                return Err(format_err!("scsi command failed with err {}", err).into());
+                Err(format_err!("scsi command failed with err {}", err).into())
             }
-            unknown => return Err(format_err!("scsi command failed: unknown result category {}", unknown).into()),
+            unknown => Err(format_err!("scsi command failed: unknown result category {}", unknown).into()),
         }
     }
 
@@ -557,7 +557,7 @@ impl <'a, F: AsRawFd> SgRaw<'a, F> {
             return Err(format_err!("no valid SCSI command").into());
         }
 
-        if data.len() == 0 {
+        if data.is_empty() {
             return Err(format_err!("got zero-sized input buffer").into());
         }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 11/14] config/tape_encryption_keys: clippy fixes

[Dominik Csapak]

fixes:
* combine if branches
* unnecessary clone

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/config/tape_encryption_keys.rs | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/config/tape_encryption_keys.rs b/src/config/tape_encryption_keys.rs
index 42c3184d..db3c9fbf 100644
--- a/src/config/tape_encryption_keys.rs
+++ b/src/config/tape_encryption_keys.rs
@@ -201,17 +201,15 @@ pub fn insert_key(key: [u8;32], key_config: KeyConfig, force: bool) -> Result<()
         None => bail!("missing encryption key fingerprint - internal error"),
     };
 
-    if !force {
-        if config_map.get(&fingerprint).is_some() {
-            bail!("encryption key '{}' already exists.", fingerprint);
-        }
+    if !force && config_map.get(&fingerprint).is_some() {
+        bail!("encryption key '{}' already exists.", fingerprint);
     }
 
     let item = EncryptionKeyInfo::new(key, fingerprint.clone());
     key_map.insert(fingerprint.clone(), item);
     save_keys(key_map)?;
 
-    config_map.insert(fingerprint.clone(), key_config);
+    config_map.insert(fingerprint, key_config);
     save_key_configs(config_map)?;
 
     Ok(())
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 12/14] server/worker_task: clippy fix

[Dominik Csapak]

use '.to_string()' instead of 'format!()' for static str

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/server/worker_task.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs
index 6c5456c9..3e76fad0 100644
--- a/src/server/worker_task.rs
+++ b/src/server/worker_task.rs
@@ -743,7 +743,7 @@ impl WorkerTask {
 
         let prev_abort = self.abort_requested.swap(true, Ordering::SeqCst);
         if !prev_abort { // log abort one time
-            self.log(format!("received abort request ..."));
+            self.log("received abort request ...".to_string());
         }
         // noitify listeners
         let mut data = self.data.lock().unwrap();
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 13/14] bin/proxmox-file-restore: clippy fixes

[Dominik Csapak]

fixes:
* redundant_closure
* `.len() >= 0` => `!.is_empty()`
* unnecessary clone
* `if let Err(_)` => `.is_err()`

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/bin/proxmox-file-restore.rs                   | 4 +++-
 src/bin/proxmox_file_restore/block_driver_qemu.rs | 2 +-
 src/bin/proxmox_file_restore/qemu_helper.rs       | 4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox-file-restore.rs b/src/bin/proxmox-file-restore.rs
index c9ef3912..a225998c 100644
--- a/src/bin/proxmox-file-restore.rs
+++ b/src/bin/proxmox-file-restore.rs
@@ -56,7 +56,7 @@ fn parse_path(path: String, base64: bool) -> Result<ExtractPath, Error> {
         return Ok(ExtractPath::ListArchives);
     }
 
-    while bytes.len() > 0 && bytes[0] == b'/' {
+    while !bytes.is_empty() && bytes[0] == b'/' {
         bytes.remove(0);
     }
 
@@ -451,6 +451,8 @@ fn main() {
     run_cli_command(
         cmd_def,
         rpcenv,
+        // is a false positive, does not compile without the closure
+        #[allow(clippy::redundant_closure)]
         Some(|future| proxmox_backup::tools::runtime::main(future)),
     );
 }
diff --git a/src/bin/proxmox_file_restore/block_driver_qemu.rs b/src/bin/proxmox_file_restore/block_driver_qemu.rs
index 607d7d8e..a4a5e011 100644
--- a/src/bin/proxmox_file_restore/block_driver_qemu.rs
+++ b/src/bin/proxmox_file_restore/block_driver_qemu.rs
@@ -165,7 +165,7 @@ async fn ensure_running(details: &SnapRestoreDetails) -> Result<VsockClient, Err
     Ok(VsockClient::new(
         new_cid,
         DEFAULT_VSOCK_PORT,
-        Some(vms.ticket.clone()),
+        Some(vms.ticket),
     ))
 }
 
diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index 22563263..eaed3dab 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -124,10 +124,10 @@ pub async fn start_vm(
 ) -> Result<(i32, i32), Error> {
     validate_img_existance()?;
 
-    if let Err(_) = std::env::var("PBS_PASSWORD") {
+    if std::env::var("PBS_PASSWORD").is_err() {
         bail!("environment variable PBS_PASSWORD has to be set for QEMU VM restore");
     }
-    if let Err(_) = std::env::var("PBS_FINGERPRINT") {
+    if std::env::var("PBS_FINGERPRINT").is_err() {
         bail!("environment variable PBS_FINGERPRINT has to be set for QEMU VM restore");
     }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 14/14] bin/proxmox-restore-daemon: clippy fixes

[Dominik Csapak]

fixes:
* unnecessary reference
* unnecessary return
* absurd extreme comparisons

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/bin/proxmox_restore_daemon/auth.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox_restore_daemon/auth.rs b/src/bin/proxmox_restore_daemon/auth.rs
index 0973849e..1cf5a2c4 100644
--- a/src/bin/proxmox_restore_daemon/auth.rs
+++ b/src/bin/proxmox_restore_daemon/auth.rs
@@ -22,13 +22,13 @@ impl ApiAuth for StaticAuth {
         _user_info: &CachedUserInfo,
     ) -> Result<Authid, AuthError> {
         match headers.get(hyper::header::AUTHORIZATION) {
-            Some(header) if header.to_str().unwrap_or("") == &self.ticket => {
+            Some(header) if header.to_str().unwrap_or("") == self.ticket => {
                 Ok(Authid::root_auth_id().to_owned())
             }
             _ => {
-                return Err(AuthError::Generic(format_err!(
+                Err(AuthError::Generic(format_err!(
                     "invalid file restore ticket provided"
-                )));
+                )))
             }
         }
     }
@@ -38,7 +38,7 @@ pub fn ticket_auth() -> Result<StaticAuth, Error> {
     let mut ticket_file = File::open(TICKET_FILE)?;
     let mut ticket = String::new();
     let len = ticket_file.read_to_string(&mut ticket)?;
-    if len <= 0 {
+    if len == 0 {
         bail!("invalid ticket: cannot be empty");
     }
     Ok(StaticAuth { ticket })
-- 
2.20.1

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

[Dietmar Maurer]

> > 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 ...

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

[Wolfgang Bumiller]

> 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?

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

[Thomas Lamprecht]

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.

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

[Dietmar Maurer]

> > @@ -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.

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

[Wolfgang Bumiller]

> 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