[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