Re: [pbs-devel] [PATCH v1 proxmox-backup 1/3] verify-job: move building of snapshot filter into function

[Dominik Csapak <d.csapak@proxmox.com>]

while factoring that out is a good idea, there is no
need to make a builder function that returns a closure,
a much simpler diff is:

--->8---
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index a1b1e6dd..684fde67 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -575,3 +575,30 @@ pub fn verify_all_backups(

      Ok(errors)
  }
+
+/// Filter function for verification of snapshots
+pub fn verify_filter(
+    ignore_verified_snapshots: bool,
+    outdated_after: Option<i64>,
+    manifest: &BackupManifest,
+) -> bool {
+    .... old function
+    }
+}
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index 1dd8baa7..85245aca 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -7,8 +7,8 @@ use crate::{
      config::verify::VerificationJobConfig,
      backup::{
          DataStore,
-        BackupManifest,
          verify_all_backups,
+        verify_filter,
      },
      task_log,
  };
      let (email, notify) = 
crate::server::lookup_datastore_notify_settings(&verification_job.store);

      let job_id = format!("{}:{}",
@@ -68,7 +46,12 @@ pub fn do_verification_job(
              }

              let verify_worker = 
crate::backup::VerifyWorker::new(worker.clone(), datastore);
-            let result = verify_all_backups(&verify_worker, 
worker.upid(), None, Some(&filter));
+            let result = verify_all_backups(
+                &verify_worker,
+                worker.upid(),
+                None,
+                Some(&move |manifest| 
verify_filter(ignore_verified_snapshots, outdated_after, manifest)),
+            );
              let job_result = match result {
                  Ok(ref failed_dirs) if failed_dirs.is_empty() => Ok(()),
                  Ok(ref failed_dirs) => {
---8<---
(i omitted the obvious parts)

so simply have the filter function, and build
a small closure when needed

the only change this needs in 2/3 is that
let filter = build....;
must now be let filter = move |manifest| ....;


On 6/11/21 09:50, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/backup/verify.rs     | 29 +++++++++++++++++++++++++++++
>   src/server/verify_job.rs | 25 ++-----------------------
>   2 files changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
> index a1b1e6dd..a0b8de58 100644
> --- a/src/backup/verify.rs
> +++ b/src/backup/verify.rs
> @@ -1,5 +1,6 @@
>   use nix::dir::Dir;
>   use std::collections::HashSet;
> +use std::panic::RefUnwindSafe;
>   use std::sync::atomic::{AtomicUsize, Ordering};
>   use std::sync::{Arc, Mutex};
>   use std::time::Instant;
> @@ -575,3 +576,31 @@ pub fn verify_all_backups(
>   
>       Ok(errors)
>   }
> +
> +/// Builds a filter for the verification of snapshots
> +pub fn build_verify_filter(
> +    ignore_verified_snapshots: bool,
> +    outdated_after: Option<i64>,
> +) -> Box<dyn std::marker::Sync + RefUnwindSafe + Fn(&BackupManifest) -> bool> {
> +    Box::new(move |manifest: &BackupManifest| {
> +        if !ignore_verified_snapshots {
> +            return true;
> +        }
> +
> +        let raw_verify_state = manifest.unprotected["verify_state"].clone();
> +        match serde_json::from_value::<SnapshotVerifyState>(raw_verify_state) {
> +            Err(_) => true, // no last verification, always include
> +            Ok(last_verify) => {
> +                match outdated_after {
> +                    None => false, // never re-verify if ignored and no max age
> +                    Some(max_age) => {
> +                        let now = proxmox::tools::time::epoch_i64();
> +                        let days_since_last_verify = (now - last_verify.upid.starttime) / 86400;
> +
> +                        days_since_last_verify > max_age
> +                    }
> +                }
> +            }
> +        }
> +    })
> +}
> \ No newline at end of file
> diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
> index 1dd8baa7..47ac4da4 100644
> --- a/src/server/verify_job.rs
> +++ b/src/server/verify_job.rs
> @@ -7,7 +7,7 @@ use crate::{
>       config::verify::VerificationJobConfig,
>       backup::{
>           DataStore,
> -        BackupManifest,
> +        build_verify_filter,
>           verify_all_backups,
>       },
>       task_log,
> @@ -26,28 +26,6 @@ pub fn do_verification_job(
>       let outdated_after = verification_job.outdated_after;
>       let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
>   
> -    let filter = move |manifest: &BackupManifest| {
> -        if !ignore_verified_snapshots {
> -            return true;
> -        }
> -
> -        let raw_verify_state = manifest.unprotected["verify_state"].clone();
> -        match serde_json::from_value::<SnapshotVerifyState>(raw_verify_state) {
> -            Err(_) => true, // no last verification, always include
> -            Ok(last_verify) => {
> -                match outdated_after {
> -                    None => false, // never re-verify if ignored and no max age
> -                    Some(max_age) => {
> -                        let now = proxmox::tools::time::epoch_i64();
> -                        let days_since_last_verify = (now - last_verify.upid.starttime) / 86400;
> -
> -                        days_since_last_verify > max_age
> -                    }
> -                }
> -            }
> -        }
> -    };
> -
>       let (email, notify) = crate::server::lookup_datastore_notify_settings(&verification_job.store);
>   
>       let job_id = format!("{}:{}",
> @@ -67,6 +45,7 @@ pub fn do_verification_job(
>                   task_log!(worker,"task triggered by schedule '{}'", event_str);
>               }
>   
> +            let filter = build_verify_filter(ignore_verified_snapshots, outdated_after);
>               let verify_worker = crate::backup::VerifyWorker::new(worker.clone(), datastore);
>               let result = verify_all_backups(&verify_worker, worker.upid(), None, Some(&filter));
>               let job_result = match result {
>