[pbs-devel] [PATCH proxmox-backup 0/8] clippy fixes

[pbs-devel] [PATCH proxmox-backup 0/8] clippy fixes

[Fabian Grünbichler]

non-trivial clippy fixes and stuff I found while fixing clippy issues.
the remainder of warnings are now:

- one derive_hash_xor_eq (Authid)
- one module_inception (rrd)
- one missing_safety_docs
- one wrong_self_convention (to_canonical_json not taking &self)
- plenty complex types, too many arguments and missing Default impl when
types have a fn new() without args

which require individual attention to decide whether to refactor/fix, or
allow them.

when writing new commits, the following should cut down on the noise
until all of the above are addressed:

$ cargo clippy -- --allow clippy::type_complexity --allow clippy::new_without_default --allow clippy::too_many_arguments

Fabian Grünbichler (8):
  clippy: add is_empty() when len() is implemented
  clippy: fix Mutex with unused value
  clippy: rewrite comparison chains
  clippy: rewrite ifs with identical return values
  apt: let api handle optional bool with default
  rework GC traversal error handling
  http-client: fix typoed ticket cache condition
  http-client: further clippy cleanups

 src/api2/access.rs             | 32 +++++++++++++-------------------
 src/api2/backup/environment.rs |  4 +++-
 src/api2/node/apt.rs           |  7 ++-----
 src/api2/node/dns.rs           |  2 +-
 src/api2/node/tasks.rs         | 16 ++++++----------
 src/backup/chunk_store.rs      |  4 ++--
 src/backup/chunk_stream.rs     |  7 ++-----
 src/backup/datastore.rs        | 18 ++++++++++--------
 src/client/http_client.rs      | 10 +++++-----
 src/config/network/parser.rs   |  1 +
 src/tools/acl.rs               |  9 +++++++--
 src/tools/lru_cache.rs         |  5 +++++
 12 files changed, 57 insertions(+), 58 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/8] clippy: add is_empty() when len() is implemented

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/tools/acl.rs       | 3 +++
 src/tools/lru_cache.rs | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/src/tools/acl.rs b/src/tools/acl.rs
index d9ae42a0..94f3f4df 100644
--- a/src/tools/acl.rs
+++ b/src/tools/acl.rs
@@ -322,6 +322,9 @@ impl ACLXAttrBuffer {
         self.buffer.len()
     }
 
+    /// The buffer always contains at least the version, it is never empty
+    pub const fn is_empty(&self) -> bool { false }
+
     /// Borrow raw buffer as mut slice.
     pub fn as_mut_slice(&mut self) -> &mut [u8] {
         self.buffer.as_mut_slice()
diff --git a/src/tools/lru_cache.rs b/src/tools/lru_cache.rs
index 19012976..ecd15ba6 100644
--- a/src/tools/lru_cache.rs
+++ b/src/tools/lru_cache.rs
@@ -184,6 +184,11 @@ impl<K: std::cmp::Eq + std::hash::Hash + Copy, V> LruCache<K, V> {
         self.map.len()
     }
 
+    /// Returns `true` when the cache is empty
+    pub fn is_empty(&self) -> bool {
+        self.map.is_empty()
+    }
+
     /// Get a mutable reference to the value identified by `key`.
     /// This will update the cache entry to be the most recently used entry.
     /// On cache misses, the cachers fetch method is called to get a corresponding
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/8] clippy: fix Mutex with unused value

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/node/dns.rs      | 2 +-
 src/backup/chunk_store.rs | 4 ++--
 src/backup/datastore.rs   | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/api2/node/dns.rs b/src/api2/node/dns.rs
index 9a22bf60..e3dd614d 100644
--- a/src/api2/node/dns.rs
+++ b/src/api2/node/dns.rs
@@ -125,7 +125,7 @@ pub fn update_dns(
 ) -> Result<Value, Error> {
 
     lazy_static! {
-        static ref MUTEX: Arc<Mutex<usize>> = Arc::new(Mutex::new(0));
+        static ref MUTEX: Arc<Mutex<()>> = Arc::new(Mutex::new(()));
     }
 
     let _guard = MUTEX.lock();
diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
index f782169d..cc4a9435 100644
--- a/src/backup/chunk_store.rs
+++ b/src/backup/chunk_store.rs
@@ -18,7 +18,7 @@ pub struct ChunkStore {
     name: String, // used for error reporting
     pub (crate) base: PathBuf,
     chunk_dir: PathBuf,
-    mutex: Mutex<bool>,
+    mutex: Mutex<()>,
     locker: Arc<Mutex<tools::ProcessLocker>>,
 }
 
@@ -143,7 +143,7 @@ impl ChunkStore {
             base,
             chunk_dir,
             locker,
-            mutex: Mutex::new(false)
+            mutex: Mutex::new(())
         })
     }
 
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index b1099460..34865380 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -37,7 +37,7 @@ lazy_static! {
 /// management interface for backup.
 pub struct DataStore {
     chunk_store: Arc<ChunkStore>,
-    gc_mutex: Mutex<bool>,
+    gc_mutex: Mutex<()>,
     last_gc_status: Mutex<GarbageCollectionStatus>,
     verify_new: bool,
 }
@@ -89,7 +89,7 @@ impl DataStore {
 
         Ok(Self {
             chunk_store: Arc::new(chunk_store),
-            gc_mutex: Mutex::new(false),
+            gc_mutex: Mutex::new(()),
             last_gc_status: Mutex::new(gc_status),
             verify_new: config.verify_new.unwrap_or(false),
         })
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/8] clippy: rewrite comparison chains

[Fabian Grünbichler]

chunk_stream one can be collapsed, since split == split_to with at set
to buffer.len() anyway.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/backup/environment.rs | 4 +++-
 src/backup/chunk_stream.rs     | 7 ++-----
 src/config/network/parser.rs   | 1 +
 src/tools/acl.rs               | 6 ++++--
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 53fd76a2..38061816 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -185,7 +185,9 @@ impl BackupEnvironment {
 
         if size > data.chunk_size {
             bail!("fixed writer '{}' - got large chunk ({} > {}", data.name, size, data.chunk_size);
-        } else if size < data.chunk_size {
+        }
+
+        if size < data.chunk_size {
             data.small_chunk_count += 1;
             if data.small_chunk_count > 1 {
                 bail!("fixed writer '{}' - detected multiple end chunks (chunk size too small)");
diff --git a/src/backup/chunk_stream.rs b/src/backup/chunk_stream.rs
index 2c4040c4..dd37832b 100644
--- a/src/backup/chunk_stream.rs
+++ b/src/backup/chunk_stream.rs
@@ -98,11 +98,8 @@ where
     fn poll_next(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Option<Result<BytesMut, S::Error>>> {
         let this = self.get_mut();
         loop {
-            if this.buffer.len() == this.chunk_size {
-                return Poll::Ready(Some(Ok(this.buffer.split())));
-            } else if this.buffer.len() > this.chunk_size {
-                let result = this.buffer.split_to(this.chunk_size);
-                return Poll::Ready(Some(Ok(result)));
+            if this.buffer.len() >= this.chunk_size {
+                return Poll::Ready(Some(Ok(this.buffer.split_to(this.chunk_size))));
             }
 
             match ready!(Pin::new(&mut this.input).try_poll_next(cx)) {
diff --git a/src/config/network/parser.rs b/src/config/network/parser.rs
index 2546f027..ff36a314 100644
--- a/src/config/network/parser.rs
+++ b/src/config/network/parser.rs
@@ -293,6 +293,7 @@ impl <R: BufRead> NetworkParser<R> {
             }
         }
 
+        #[allow(clippy::comparison_chain)]
         if let Some(netmask) = netmask {
             if address_list.len() > 1 {
                 bail!("unable to apply netmask to multiple addresses (please use cidr notation)");
diff --git a/src/tools/acl.rs b/src/tools/acl.rs
index 94f3f4df..80e27812 100644
--- a/src/tools/acl.rs
+++ b/src/tools/acl.rs
@@ -196,9 +196,11 @@ impl<'a> ACLEntry<'a> {
 
         for &perm in &[ACL_READ, ACL_WRITE, ACL_EXECUTE] {
             res = unsafe { acl_get_perm(permset, perm) };
-            if res < 0 { 
+            if res < 0 {
                 return Err(Errno::last());
-            } else if res > 0 { 
+            }
+
+            if res == 1 {
                 permissions |= perm as u64;
             }
         }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/8] clippy: rewrite ifs with identical return values

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/access.rs     | 32 +++++++++++++-------------------
 src/api2/node/tasks.rs | 16 ++++++----------
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/src/api2/access.rs b/src/api2/access.rs
index 61d0f74e..b4d78395 100644
--- a/src/api2/access.rs
+++ b/src/api2/access.rs
@@ -25,6 +25,7 @@ pub mod role;
 pub mod tfa;
 pub mod user;
 
+#[allow(clippy::large_enum_variant)]
 enum AuthResult {
     /// Successful authentication which does not require a new ticket.
     Success,
@@ -329,27 +330,20 @@ pub fn list_permissions(
     let user_info = CachedUserInfo::new()?;
     let user_privs = user_info.lookup_privs(&current_auth_id, &["access"]);
 
-    let auth_id = if user_privs & PRIV_SYS_AUDIT == 0 {
-        match auth_id {
-            Some(auth_id) => {
-                if auth_id == current_auth_id {
-                    auth_id
-                } else if auth_id.is_token()
+    let auth_id = match auth_id {
+        Some(auth_id) if auth_id == current_auth_id => current_auth_id,
+        Some(auth_id) => {
+            if user_privs & PRIV_SYS_AUDIT != 0 
+                || (auth_id.is_token()
                     && !current_auth_id.is_token()
-                    && auth_id.user() == current_auth_id.user()
-                {
-                    auth_id
-                } else {
-                    bail!("not allowed to list permissions of {}", auth_id);
-                }
+                    && auth_id.user() == current_auth_id.user())
+            {
+                auth_id
+            } else {
+                bail!("not allowed to list permissions of {}", auth_id);
             }
-            None => current_auth_id,
-        }
-    } else {
-        match auth_id {
-            Some(auth_id) => auth_id,
-            None => current_auth_id,
-        }
+        },
+        None => current_auth_id,
     };
 
     fn populate_acl_paths(
diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index 8de35ca9..99470531 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -110,16 +110,12 @@ fn check_task_access(auth_id: &Authid, upid: &UPID) -> Result<(), Error> {
     } else {
         let user_info = CachedUserInfo::new()?;
 
-        let task_privs = user_info.lookup_privs(auth_id, &["system", "tasks"]);
-        if task_privs & PRIV_SYS_AUDIT != 0 {
-            // allowed to read all tasks in general
-            Ok(())
-        } else if check_job_privs(&auth_id, &user_info, upid).is_ok() {
-            // job which the user/token could have configured/manually executed
-            Ok(())
-        } else {
-            bail!("task access not allowed");
-        }
+        // access to all tasks
+        // or task == job which the user/token could have configured/manually executed
+
+        user_info.check_privs(auth_id, &["system", "tasks"], PRIV_SYS_AUDIT, false)
+            .or_else(|_| check_job_privs(&auth_id, &user_info, upid))
+            .or_else(|_| bail!("task access not allowed"))
     }
 }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/8] apt: let api handle optional bool with default

[Fabian Grünbichler]

one less FIXME :)

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/node/apt.rs | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
index 52eeb976..9d723f17 100644
--- a/src/api2/node/apt.rs
+++ b/src/api2/node/apt.rs
@@ -107,16 +107,13 @@ fn do_apt_update(worker: &WorkerTask, quiet: bool) -> Result<(), Error> {
 )]
 /// Update the APT database
 pub fn apt_update_database(
-    notify: Option<bool>,
-    quiet: Option<bool>,
+    notify: bool,
+    quiet: bool,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
-    // FIXME: change to non-option in signature and drop below once we have proxmox-api-macro 0.2.3
-    let quiet = quiet.unwrap_or(API_METHOD_APT_UPDATE_DATABASE_PARAM_DEFAULT_QUIET);
-    let notify = notify.unwrap_or(API_METHOD_APT_UPDATE_DATABASE_PARAM_DEFAULT_NOTIFY);
 
     let upid_str = WorkerTask::new_thread("aptupdate", None, auth_id, to_stdout, move |worker| {
         do_apt_update(&worker, quiet)?;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 6/8] rework GC traversal error handling

[Fabian Grünbichler]

the error message don't make sense with an empty default

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/datastore.rs | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 34865380..fae7ff25 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -395,16 +395,18 @@ impl DataStore {
         }
         let handle_entry_err = |err: walkdir::Error| {
             if let Some(inner) = err.io_error() {
-                let path = err.path().unwrap_or(Path::new(""));
-                match inner.kind() {
-                    io::ErrorKind::PermissionDenied => {
+                if let Some(path) = err.path() {
+                    if inner.kind() == io::ErrorKind::PermissionDenied {
                         // only allow to skip ext4 fsck directory, avoid GC if, for example,
                         // a user got file permissions wrong on datastore rsync to new server
                         if err.depth() > 1 || !path.ends_with("lost+found") {
-                            bail!("cannot continue garbage-collection safely, permission denied on: {}", path.display())
+                            bail!("cannot continue garbage-collection safely, permission denied on: {:?}", path)
                         }
-                    },
-                    _ => bail!("unexpected error on datastore traversal: {} - {}", inner, path.display()),
+                    } else {
+                        bail!("unexpected error on datastore traversal: {} - {:?}", inner, path)
+                    }
+                } else {
+                    bail!("unexpected error on datastore traversal: {}", inner)
                 }
             }
             Ok(())
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 7/8] http-client: fix typoed ticket cache condition

[Fabian Grünbichler]

which was even copy-pasted once without noticing.

found with clippy.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/client/http_client.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index 3a934062..567a6b73 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -351,7 +351,7 @@ impl HttpClient {
                 };
                 match Self::credentials(client2.clone(), server2.clone(), port, auth_id.user().clone(), ticket).await {
                     Ok(auth) => {
-                        if use_ticket_cache & &prefix2.is_some() {
+                        if use_ticket_cache && prefix2.is_some() {
                             let _ = store_ticket_info(prefix2.as_ref().unwrap(), &server2, &auth.auth_id.to_string(), &auth.ticket, &auth.token);
                         }
                         *auth2.write().unwrap() = auth;
@@ -378,7 +378,7 @@ impl HttpClient {
             let authinfo = auth.clone();
 
             move |auth| {
-                if use_ticket_cache & &prefix.is_some() {
+                if use_ticket_cache && prefix.is_some() {
                     let _ = store_ticket_info(prefix.as_ref().unwrap(), &server, &auth.auth_id.to_string(), &auth.ticket, &auth.token);
                 }
                 *authinfo.write().unwrap() = auth;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 8/8] http-client: further clippy cleanups

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/client/http_client.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index 567a6b73..f279d9dd 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -156,7 +156,7 @@ fn store_fingerprint(prefix: &str, server: &str, fingerprint: &str) -> Result<()
     raw.split('\n').for_each(|line| {
         let items: Vec<String> = line.split_whitespace().map(String::from).collect();
         if items.len() == 2 {
-            if &items[0] == server {
+            if items[0] == server {
                 // found, add later with new fingerprint
             } else {
                 result.push_str(line);
@@ -186,7 +186,7 @@ fn load_fingerprint(prefix: &str, server: &str) -> Option<String> {
 
     for line in raw.split('\n') {
         let items: Vec<String> = line.split_whitespace().map(String::from).collect();
-        if items.len() == 2 && &items[0] == server {
+        if items.len() == 2 && items[0] == server {
             return Some(items[1].clone());
         }
     }
@@ -371,7 +371,7 @@ impl HttpClient {
             server.to_owned(),
             port,
             auth_id.user().clone(),
-            password.to_owned(),
+            password,
         ).map_ok({
             let server = server.to_string();
             let prefix = options.prefix.clone();
-- 
2.20.1

[pbs-devel] applied series: [PATCH proxmox-backup 0/8] clippy fixes

[Wolfgang Bumiller]

applied series

On Wed, Jan 20, 2021 at 05:23:47PM +0100, Fabian Grünbichler wrote:
> non-trivial clippy fixes and stuff I found while fixing clippy issues.
> the remainder of warnings are now:
> 
> - one derive_hash_xor_eq (Authid)
> - one module_inception (rrd)
> - one missing_safety_docs
> - one wrong_self_convention (to_canonical_json not taking &self)
> - plenty complex types, too many arguments and missing Default impl when
> types have a fn new() without args
> 
> which require individual attention to decide whether to refactor/fix, or
> allow them.
> 
> when writing new commits, the following should cut down on the noise
> until all of the above are addressed:
> 
> $ cargo clippy -- --allow clippy::type_complexity --allow clippy::new_without_default --allow clippy::too_many_arguments
> 
> Fabian Grünbichler (8):
>   clippy: add is_empty() when len() is implemented
>   clippy: fix Mutex with unused value
>   clippy: rewrite comparison chains
>   clippy: rewrite ifs with identical return values
>   apt: let api handle optional bool with default
>   rework GC traversal error handling
>   http-client: fix typoed ticket cache condition
>   http-client: further clippy cleanups
> 
>  src/api2/access.rs             | 32 +++++++++++++-------------------
>  src/api2/backup/environment.rs |  4 +++-
>  src/api2/node/apt.rs           |  7 ++-----
>  src/api2/node/dns.rs           |  2 +-
>  src/api2/node/tasks.rs         | 16 ++++++----------
>  src/backup/chunk_store.rs      |  4 ++--
>  src/backup/chunk_stream.rs     |  7 ++-----
>  src/backup/datastore.rs        | 18 ++++++++++--------
>  src/client/http_client.rs      | 10 +++++-----
>  src/config/network/parser.rs   |  1 +
>  src/tools/acl.rs               |  9 +++++++--
>  src/tools/lru_cache.rs         |  5 +++++
>  12 files changed, 57 insertions(+), 58 deletions(-)
> 
> -- 
> 2.20.1