[pbs-devel] [PATCH backup 1/4] disks: remove useless conversion to the same type

[pbs-devel] [PATCH backup 1/4] disks: remove useless conversion to the same type

[Maximiliano Sandoval]

Fixes the clippy lint:

```
warning: useless conversion to the same type: `std::ffi::OsString`
    --> src/tools/disks/mod.rs:1161:9
     |
1161 |         count_str.into(),
     |         ^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `count_str`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
```

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
This patch series contains the missing patches from my "last" clippy patch series.

 src/tools/disks/mod.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 78360b66..94f89e0a 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -1158,7 +1158,7 @@ pub fn wipe_blockdev(disk: &Disk, worker: Arc<WorkerTask>) -> Result<(), Error>
         of_path,
         "bs=1M".into(),
         "conv=fdatasync".into(),
-        count_str.into(),
+        count_str,
     ];
     dd_command.args(args);
 
-- 
2.39.2

[pbs-devel] [PATCH backup 2/4] backup-proxy: avoid block in if condition

[Maximiliano Sandoval]

Fixes the clippy lint:

```
warning: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
   --> src/bin/proxmox-backup-proxy.rs:874:58
    |
874 |           let stats = match tokio::task::spawn_blocking(|| {
    |  __________________________________________________________^
875 | |             let hoststats = collect_host_stats_sync();
876 | |             let (hostdisk, datastores) = collect_disk_stats_sync();
877 | |             Arc::new((hoststats, hostdisk, datastores))
878 | |         })
    | |_________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions
    = note: `#[warn(clippy::blocks_in_conditions)]` on by default
```

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

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 9c49026b..b6e1fdd7 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -871,13 +871,12 @@ async fn run_stat_generator() {
     loop {
         let delay_target = Instant::now() + Duration::from_secs(10);
 
-        let stats = match tokio::task::spawn_blocking(|| {
+        let stats_res = tokio::task::spawn_blocking(|| {
             let hoststats = collect_host_stats_sync();
             let (hostdisk, datastores) = collect_disk_stats_sync();
             Arc::new((hoststats, hostdisk, datastores))
-        })
-        .await
-        {
+        });
+        let stats = match stats_res.await {
             Ok(res) => res,
             Err(err) => {
                 log::error!("collecting host stats panicked: {err}");
-- 
2.39.2

[pbs-devel] [PATCH backup 3/4] report: inline errors in writeln!

[Maximiliano Sandoval]

Fixes the clippy lint:

```
warning: `to_string` applied to a type that implements `Display` in `writeln!` args
   --> src/server/report.rs:141:72
    |
141 |                 let _ = writeln!(out, "error during read-dir - {}", err.to_string());
    |                                                                        ^^^^^^^^^^^^ help: remove this
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
```

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

diff --git a/src/server/report.rs b/src/server/report.rs
index d97efb14..ca5f04fc 100644
--- a/src/server/report.rs
+++ b/src/server/report.rs
@@ -126,9 +126,8 @@ fn get_directory_content(path: impl AsRef<Path>) -> String {
         Ok(iter) => iter,
         Err(err) => {
             return format!(
-                "`$ cat '{}*'`\n```\n# read dir failed - {}\n```",
+                "`$ cat '{}*'`\n```\n# read dir failed - {err}\n```",
                 path.as_ref().display(),
-                err.to_string(),
             );
         }
     };
@@ -138,7 +137,7 @@ fn get_directory_content(path: impl AsRef<Path>) -> String {
         let entry = match entry {
             Ok(entry) => entry,
             Err(err) => {
-                let _ = writeln!(out, "error during read-dir - {}", err.to_string());
+                let _ = writeln!(out, "error during read-dir - {err}");
                 continue;
             }
         };
-- 
2.39.2

[pbs-devel] [PATCH backup 4/4] api: use if-let pattern for error-only handling

[Maximiliano Sandoval]

It is more readable than using match. We also inline variables in
eprintln!.

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

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index 118838ce..035c8a57 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -391,18 +391,12 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
         }
     }
 
-    match crate::config::tfa::read().and_then(|mut cfg| {
+    if let Err(err) = crate::config::tfa::read().and_then(|mut cfg| {
         let _: proxmox_tfa::api::NeedsSaving =
             cfg.remove_user(&crate::config::tfa::UserAccess, userid.as_str())?;
         crate::config::tfa::write(&cfg)
     }) {
-        Ok(()) => (),
-        Err(err) => {
-            eprintln!(
-                "error updating TFA config after deleting user {:?}: {}",
-                userid, err
-            );
-        }
+        eprintln!("error updating TFA config after deleting user {userid:?} {err}",);
     }
 
     Ok(())
-- 
2.39.2

Re: [pbs-devel] [PATCH backup 4/4] api: use if-let pattern for error-only handling

[Fabian Grünbichler]

On February 13, 2024 10:53 am, Maximiliano Sandoval wrote:
> It is more readable than using match. We also inline variables in
> eprintln!.

what about the exact same pattern directly above it? there's like 10
more places to look at based on simple grepping..

> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/api2/access/user.rs | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index 118838ce..035c8a57 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -391,18 +391,12 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
>          }
>      }
>  
> -    match crate::config::tfa::read().and_then(|mut cfg| {
> +    if let Err(err) = crate::config::tfa::read().and_then(|mut cfg| {
>          let _: proxmox_tfa::api::NeedsSaving =
>              cfg.remove_user(&crate::config::tfa::UserAccess, userid.as_str())?;
>          crate::config::tfa::write(&cfg)
>      }) {
> -        Ok(()) => (),
> -        Err(err) => {
> -            eprintln!(
> -                "error updating TFA config after deleting user {:?}: {}",
> -                userid, err
> -            );
> -        }
> +        eprintln!("error updating TFA config after deleting user {userid:?} {err}",);
>      }
>  
>      Ok(())
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [PATCH backup 2/4] backup-proxy: avoid block in if condition

[Fabian Grünbichler]

On February 13, 2024 10:53 am, Maximiliano Sandoval wrote:
> Fixes the clippy lint:
> 
> ```
> warning: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
>    --> src/bin/proxmox-backup-proxy.rs:874:58
>     |
> 874 |           let stats = match tokio::task::spawn_blocking(|| {
>     |  __________________________________________________________^
> 875 | |             let hoststats = collect_host_stats_sync();
> 876 | |             let (hostdisk, datastores) = collect_disk_stats_sync();
> 877 | |             Arc::new((hoststats, hostdisk, datastores))
> 878 | |         })
>     | |_________^
>     |
>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions
>     = note: `#[warn(clippy::blocks_in_conditions)]` on by default
> ```
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/bin/proxmox-backup-proxy.rs | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 9c49026b..b6e1fdd7 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -871,13 +871,12 @@ async fn run_stat_generator() {
>      loop {
>          let delay_target = Instant::now() + Duration::from_secs(10);
>  
> -        let stats = match tokio::task::spawn_blocking(|| {
> +        let stats_res = tokio::task::spawn_blocking(|| {

nit: called _res , but this is actually the future.. either stats_future, or await it
directly instead of below.

it could also be somewhat simplified by doing

match stats_res {
    Ok(stats) => { // do all the broadcasting here },
    Err(err) => { // log error here },
};

// sleep here

but that also increases the level of nesting for the broadcasting part,
so it's not a 100% win ;)

>              let hoststats = collect_host_stats_sync();
>              let (hostdisk, datastores) = collect_disk_stats_sync();
>              Arc::new((hoststats, hostdisk, datastores))
> -        })
> -        .await
> -        {
> +        });
> +        let stats = match stats_res.await {
>              Ok(res) => res,
>              Err(err) => {
>                  log::error!("collecting host stats panicked: {err}");
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] partially-applied: [PATCH backup 1/4] disks: remove useless conversion to the same type

[Fabian Grünbichler]

applied 1 and 3, comments for 2 and 4 sent as well..

On February 13, 2024 10:53 am, Maximiliano Sandoval wrote:
> Fixes the clippy lint:
> 
> ```
> warning: useless conversion to the same type: `std::ffi::OsString`
>     --> src/tools/disks/mod.rs:1161:9
>      |
> 1161 |         count_str.into(),
>      |         ^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `count_str`
>      |
>      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
> ```
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> This patch series contains the missing patches from my "last" clippy patch series.
> 
>  src/tools/disks/mod.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index 78360b66..94f89e0a 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -1158,7 +1158,7 @@ pub fn wipe_blockdev(disk: &Disk, worker: Arc<WorkerTask>) -> Result<(), Error>
>          of_path,
>          "bs=1M".into(),
>          "conv=fdatasync".into(),
> -        count_str.into(),
> +        count_str,
>      ];
>      dd_command.args(args);
>  
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>