[pbs-devel] [PATCH v2 backup 0/5] Clippy fixes

[pbs-devel] [PATCH v2 backup 0/5] Clippy fixes

[Maximiliano Sandoval]

Differences from v1:

- Rebased on top of master
- Changed prefix to backup
- Added space in commits between description and signed by
- Changed commit descriptions to include the "Why is it bad" blurb instead of
  the URL, with the exception of the self-explanatory cast to i64

Maximiliano Sandoval (5):
  clippy fixes: the borrowed expression implements the required traits
  clippy fixes: use of ok_or followed by a function call
  clippy fixes: casting to the same type is unnecessary
  clippy fixes: Box::new(_) of default value
  clippy fixes: deref which would be done by auto-deref

 src/acme/plugin.rs             | 2 +-
 src/api2/admin/datastore.rs    | 2 +-
 src/api2/config/acme.rs        | 4 ++--
 src/api2/node/services.rs      | 4 +++-
 src/api2/tape/drive.rs         | 2 +-
 src/api2/tape/restore.rs       | 2 +-
 src/auth.rs                    | 6 +++---
 src/config/mod.rs              | 4 ++--
 src/config/tfa.rs              | 2 +-
 src/server/jobstate.rs         | 2 +-
 src/tape/drive/virtual_tape.rs | 8 ++++----
 src/tape/encryption_keys.rs    | 2 +-
 src/tape/media_catalog.rs      | 2 +-
 src/tape/media_pool.rs         | 4 +---
 src/tools/disks/mod.rs         | 2 +-
 src/tools/disks/zfs.rs         | 2 +-
 16 files changed, 25 insertions(+), 25 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH v2 backup 1/5] clippy fixes: the borrowed expression implements the required traits

[Maximiliano Sandoval]

Suggests that the receiver of the expression borrows the expression.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/api2/admin/datastore.rs    | 2 +-
 src/api2/config/acme.rs        | 4 ++--
 src/api2/tape/restore.rs       | 2 +-
 src/auth.rs                    | 2 +-
 src/config/mod.rs              | 4 ++--
 src/config/tfa.rs              | 2 +-
 src/server/jobstate.rs         | 2 +-
 src/tape/drive/virtual_tape.rs | 8 ++++----
 src/tape/encryption_keys.rs    | 2 +-
 src/tape/media_catalog.rs      | 2 +-
 src/tools/disks/mod.rs         | 2 +-
 src/tools/disks/zfs.rs         | 2 +-
 12 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 3551e2f0..a95031e7 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -237,7 +237,7 @@ pub fn list_groups(
                 .to_owned();
 
             let note_path = get_group_note_path(&datastore, &ns, group.as_ref());
-            let comment = file_read_firstline(&note_path).ok();
+            let comment = file_read_firstline(note_path).ok();
 
             group_info.push(GroupListItem {
                 backup: group.into(),
diff --git a/src/api2/config/acme.rs b/src/api2/config/acme.rs
index 3ea18493..1954318b 100644
--- a/src/api2/config/acme.rs
+++ b/src/api2/config/acme.rs
@@ -585,7 +585,7 @@ pub fn add_plugin(r#type: String, core: DnsPluginCore, data: String) -> Result<(
         param_bail!("type", "invalid ACME plugin type: {:?}", r#type);
     }
 
-    let data = String::from_utf8(base64::decode(&data)?)
+    let data = String::from_utf8(base64::decode(data)?)
         .map_err(|_| format_err!("data must be valid UTF-8"))?;
 
     let id = core.id.clone();
@@ -695,7 +695,7 @@ pub fn update_plugin(
     let (mut plugins, expected_digest) = plugin::config()?;
 
     if let Some(digest) = digest {
-        let digest = <[u8; 32]>::from_hex(&digest)?;
+        let digest = <[u8; 32]>::from_hex(digest)?;
         crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
     }
 
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 7b4fee9c..19df4a12 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -949,7 +949,7 @@ fn restore_list_worker(
 
     for (datastore, _) in store_map.used_datastores().values() {
         let tmp_path = media_set_tmpdir(datastore, &media_set_uuid);
-        match std::fs::remove_dir_all(&tmp_path) {
+        match std::fs::remove_dir_all(tmp_path) {
             Ok(()) => {}
             Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
             Err(err) => task_warn!(worker, "error cleaning up: {}", err),
diff --git a/src/auth.rs b/src/auth.rs
index 3c2654b9..432bdb2c 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -341,7 +341,7 @@ impl proxmox_auth_api::api::AuthContext for PbsAuthContext {
 
         if let Ok(Empty) = Ticket::parse(password).and_then(|ticket| {
             ticket.verify(
-                &self.keyring,
+                self.keyring,
                 TERM_PREFIX,
                 Some(&crate::tools::ticket::term_aad(userid, &path, port)),
             )
diff --git a/src/config/mod.rs b/src/config/mod.rs
index e86802ab..00e42dac 100644
--- a/src/config/mod.rs
+++ b/src/config/mod.rs
@@ -187,9 +187,9 @@ pub(crate) fn set_proxy_certificate(cert_pem: &[u8], key_pem: &[u8]) -> Result<(
     let cert_path = PathBuf::from(configdir!("/proxy.pem"));
 
     create_configdir()?;
-    pbs_config::replace_backup_config(&key_path, key_pem)
+    pbs_config::replace_backup_config(key_path, key_pem)
         .map_err(|err| format_err!("error writing certificate private key - {}", err))?;
-    pbs_config::replace_backup_config(&cert_path, cert_pem)
+    pbs_config::replace_backup_config(cert_path, cert_pem)
         .map_err(|err| format_err!("error writing certificate file - {}", err))?;
 
     Ok(())
diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index 6b1312bb..9c322abd 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -280,7 +280,7 @@ impl proxmox_tfa::api::OpenUserChallengeData for UserAccess {
     /// `remove` user data if it exists.
     fn remove(&self, userid: &str) -> Result<bool, Error> {
         let path = challenge_data_path_str(userid);
-        match std::fs::remove_file(&path) {
+        match std::fs::remove_file(path) {
             Ok(()) => Ok(true),
             Err(err) if err.not_found() => Ok(false),
             Err(err) => Err(err.into()),
diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
index 3f0acb8c..a13d768b 100644
--- a/src/server/jobstate.rs
+++ b/src/server/jobstate.rs
@@ -229,7 +229,7 @@ impl Job {
     pub fn new(jobtype: &str, jobname: &str) -> Result<Self, Error> {
         let path = get_path(jobtype, jobname);
 
-        let _lock = get_lock(&path)?;
+        let _lock = get_lock(path)?;
 
         Ok(Self {
             jobtype: jobtype.to_string(),
diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
index d3b7b0f3..b13c58c4 100644
--- a/src/tape/drive/virtual_tape.rs
+++ b/src/tape/drive/virtual_tape.rs
@@ -90,7 +90,7 @@ impl VirtualTapeHandle {
 
     fn load_tape_index(&self, tape_name: &str) -> Result<TapeIndex, Error> {
         let path = self.tape_index_path(tape_name);
-        let raw = proxmox_sys::fs::file_get_contents(&path)?;
+        let raw = proxmox_sys::fs::file_get_contents(path)?;
         if raw.is_empty() {
             return Ok(TapeIndex { files: 0 });
         }
@@ -103,7 +103,7 @@ impl VirtualTapeHandle {
         let raw = serde_json::to_string_pretty(&serde_json::to_value(index)?)?;
 
         let options = CreateOptions::new();
-        replace_file(&path, raw.as_bytes(), options, false)?;
+        replace_file(path, raw.as_bytes(), options, false)?;
         Ok(())
     }
 
@@ -131,7 +131,7 @@ impl VirtualTapeHandle {
 
         let default = serde_json::to_value(VirtualDriveStatus { current_tape: None })?;
 
-        let data = proxmox_sys::fs::file_get_json(&path, Some(default))?;
+        let data = proxmox_sys::fs::file_get_json(path, Some(default))?;
         let status: VirtualDriveStatus = serde_json::from_value(data)?;
         Ok(status)
     }
@@ -141,7 +141,7 @@ impl VirtualTapeHandle {
         let raw = serde_json::to_string_pretty(&serde_json::to_value(status)?)?;
 
         let options = CreateOptions::new();
-        replace_file(&path, raw.as_bytes(), options, false)?;
+        replace_file(path, raw.as_bytes(), options, false)?;
         Ok(())
     }
 
diff --git a/src/tape/encryption_keys.rs b/src/tape/encryption_keys.rs
index f9fdccd4..bc6dee3b 100644
--- a/src/tape/encryption_keys.rs
+++ b/src/tape/encryption_keys.rs
@@ -38,7 +38,7 @@ mod hex_key {
         D: Deserializer<'de>,
     {
         let s = String::deserialize(deserializer)?;
-        <[u8; 32]>::from_hex(&s).map_err(serde::de::Error::custom)
+        <[u8; 32]>::from_hex(s).map_err(serde::de::Error::custom)
     }
 }
 
diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
index 3b182a8d..928d4701 100644
--- a/src/tape/media_catalog.rs
+++ b/src/tape/media_catalog.rs
@@ -254,7 +254,7 @@ impl MediaCatalog {
             .write(true)
             .create(true)
             .truncate(true)
-            .open(&tmp_path)?;
+            .open(tmp_path)?;
 
         if cfg!(test) {
             // We cannot use chown inside test environment (no permissions)
diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 31ce5acb..72e374ca 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -108,7 +108,7 @@ impl DiskManage {
     /// Get a `Disk` for a name in `/sys/block/<name>`.
     pub fn disk_by_name(self: Arc<Self>, name: &str) -> io::Result<Disk> {
         let syspath = format!("/sys/block/{}", name);
-        self.disk_by_sys_path(&syspath)
+        self.disk_by_sys_path(syspath)
     }
 
     /// Gather information about mounted disks:
diff --git a/src/tools/disks/zfs.rs b/src/tools/disks/zfs.rs
index 45df81d9..b12a948b 100644
--- a/src/tools/disks/zfs.rs
+++ b/src/tools/disks/zfs.rs
@@ -122,7 +122,7 @@ lazy_static::lazy_static! {
 fn parse_objset_stat(pool: &str, objset_id: &str) -> Result<(String, BlockDevStat), Error> {
     let path = PathBuf::from(format!("{}/{}/{}", ZFS_KSTAT_BASE_PATH, pool, objset_id));
 
-    let text = match proxmox_sys::fs::file_read_optional_string(&path)? {
+    let text = match proxmox_sys::fs::file_read_optional_string(path)? {
         Some(text) => text,
         None => bail!("could not parse '{}' stat file", objset_id),
     };
-- 
2.39.2

[pbs-devel] [PATCH v2 backup 2/5] clippy fixes: use of ok_or followed by a function call

[Maximiliano Sandoval]

The function will always be called. This is only bad if it allocates or does some non-trivial amount of work.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/api2/node/services.rs | 4 +++-
 src/api2/tape/drive.rs    | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/api2/node/services.rs b/src/api2/node/services.rs
index 4a73b42d..2786d620 100644
--- a/src/api2/node/services.rs
+++ b/src/api2/node/services.rs
@@ -78,7 +78,9 @@ fn json_service_state(service: &str, status: Value) -> Value {
         let name = status["Name"].as_str().unwrap_or(service);
 
         let state = if status["Type"] == "oneshot" && status["SubState"] == "dead" {
-            status["Result"].as_str().or(status["SubState"].as_str())
+            status["Result"]
+                .as_str()
+                .or_else(|| status["SubState"].as_str())
         } else {
             status["SubState"].as_str()
         }
diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs
index 020dd492..5306e605 100644
--- a/src/api2/tape/drive.rs
+++ b/src/api2/tape/drive.rs
@@ -652,7 +652,7 @@ pub async fn read_label(drive: String, inventorize: Option<bool>) -> Result<Medi
         let mut drive = open_drive(&config, &drive)?;
 
         let (media_id, _key_config) = drive.read_label()?;
-        let media_id = media_id.ok_or(format_err!("Media is empty (no label)."))?;
+        let media_id = media_id.ok_or_else(|| format_err!("Media is empty (no label)."))?;
 
         let label = if let Some(ref set) = media_id.media_set_label {
             let key = &set.encryption_key_fingerprint;
-- 
2.39.2

[pbs-devel] [PATCH v2 backup 3/5] clippy fixes: casting to the same type is unnecessary

[Maximiliano Sandoval]

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/tape/media_pool.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/tape/media_pool.rs b/src/tape/media_pool.rs
index afb27cab..8f2b0add 100644
--- a/src/tape/media_pool.rs
+++ b/src/tape/media_pool.rs
@@ -279,9 +279,7 @@ impl MediaPool {
                         .inventory
                         .media_set_start_time(self.current_media_set.uuid())
                     {
-                        if let Ok(Some(alloc_time)) =
-                            event.compute_next_event(set_start_time as i64)
-                        {
+                        if let Ok(Some(alloc_time)) = event.compute_next_event(set_start_time) {
                             if current_time >= alloc_time {
                                 create_new_set =
                                     Some(String::from("policy CreateAt event triggered"));
-- 
2.39.2

[pbs-devel] [PATCH v2 backup 4/5] clippy fixes: Box::new(_) of default value

[Maximiliano Sandoval]

From rust-lang:

> Why is this bad?
>
> First, it’s more complex, involving two calls instead of one. Second,
> Box::default() can be faster in certain cases.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/acme/plugin.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/acme/plugin.rs b/src/acme/plugin.rs
index 478d0666..5fe8993d 100644
--- a/src/acme/plugin.rs
+++ b/src/acme/plugin.rs
@@ -35,7 +35,7 @@ pub(crate) fn get_acme_plugin(
         }
         "standalone" => {
             // this one has no config
-            Box::new(StandaloneServer::default())
+            Box::<StandaloneServer>::default()
         }
         other => bail!("missing implementation for plugin type '{}'", other),
     }))
-- 
2.39.2

Re: [pbs-devel] [PATCH v2 backup 4/5] clippy fixes: Box::new(_) of default value

[Daniele Paoni]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

Yk

In data 16 giugno 2023 10:27:35 Maximiliano Sandoval 
<m.sandoval@proxmox.com> ha scritto:

> From rust-lang:
>
>> Why is this bad?
>>
>> First, it’s more complex, involving two calls instead of one. Second,
>> Box::default() can be faster in certain cases.
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> src/acme/plugin.rs | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/acme/plugin.rs b/src/acme/plugin.rs
> index 478d0666..5fe8993d 100644
> --- a/src/acme/plugin.rs
> +++ b/src/acme/plugin.rs
> @@ -35,7 +35,7 @@ pub(crate) fn get_acme_plugin(
>         }
>         "standalone" => {
>             // this one has no config
> -            Box::new(StandaloneServer::default())
> +            Box::<StandaloneServer>::default()
>         }
>         other => bail!("missing implementation for plugin type '{}'", other),
>     }))
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


[-- Attachment #2: Type: text/html, Size: 2708 bytes --]

[pbs-devel] [PATCH v2 backup 5/5] clippy fixes: deref which would be done by auto-deref

[Maximiliano Sandoval]

This unnecessarily complicates the code.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/auth.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/auth.rs b/src/auth.rs
index 432bdb2c..e8d8d2da 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -266,11 +266,11 @@ pub fn setup_auth_context(use_private_key: bool) {
 }
 
 pub(crate) fn private_auth_keyring() -> &'static Keyring {
-    &*PRIVATE_KEYRING
+    &PRIVATE_KEYRING
 }
 
 pub(crate) fn public_auth_keyring() -> &'static Keyring {
-    &*PUBLIC_KEYRING
+    &PUBLIC_KEYRING
 }
 
 struct PbsAuthContext {
-- 
2.39.2

[pbs-devel] applied-series: [PATCH v2 backup 0/5] Clippy fixes

[Wolfgang Bumiller]

applied series, thanks

On Fri, Jun 16, 2023 at 10:27:26AM +0200, Maximiliano Sandoval wrote:
> Differences from v1:
> 
> - Rebased on top of master
> - Changed prefix to backup
> - Added space in commits between description and signed by
> - Changed commit descriptions to include the "Why is it bad" blurb instead of
>   the URL, with the exception of the self-explanatory cast to i64
> 
> Maximiliano Sandoval (5):
>   clippy fixes: the borrowed expression implements the required traits
>   clippy fixes: use of ok_or followed by a function call
>   clippy fixes: casting to the same type is unnecessary
>   clippy fixes: Box::new(_) of default value
>   clippy fixes: deref which would be done by auto-deref
> 
>  src/acme/plugin.rs             | 2 +-
>  src/api2/admin/datastore.rs    | 2 +-
>  src/api2/config/acme.rs        | 4 ++--
>  src/api2/node/services.rs      | 4 +++-
>  src/api2/tape/drive.rs         | 2 +-
>  src/api2/tape/restore.rs       | 2 +-
>  src/auth.rs                    | 6 +++---
>  src/config/mod.rs              | 4 ++--
>  src/config/tfa.rs              | 2 +-
>  src/server/jobstate.rs         | 2 +-
>  src/tape/drive/virtual_tape.rs | 8 ++++----
>  src/tape/encryption_keys.rs    | 2 +-
>  src/tape/media_catalog.rs      | 2 +-
>  src/tape/media_pool.rs         | 4 +---
>  src/tools/disks/mod.rs         | 2 +-
>  src/tools/disks/zfs.rs         | 2 +-
>  16 files changed, 25 insertions(+), 25 deletions(-)
> 
> -- 
> 2.39.2