[pbs-devel] [PATCH proxmox-backup v2] fix #3526: correctly filter tasks with 'since' and 'until'

[pbs-devel] [PATCH proxmox-backup v2] fix #3526: correctly filter tasks with 'since' and 'until'

[Dominik Csapak]

The previous assumption was that the Tasks returned by the Iterator are
sorted by the starttime, but that is not actually the case, and
could never have been, since we append the tasks into the log when
they are finished (not started) and running tasks are always iterated
first.

To correctly filter (and simplify the the api call) we forgo the
combinators, and use a for loop instead. This way we only have to do
the since/until checks only once per Task, but have to do the
start/limit counting ourselves.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* replace iterator combinators with for loop
 src/api2/node/tasks.rs | 76 ++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index 34e71af1..1602de6f 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -451,63 +451,73 @@ pub fn list_tasks(
     let list = TaskListInfoIterator::new(running)?;
     let limit = if limit > 0 { limit as usize } else { usize::MAX };
 
-    let result: Vec<TaskListItem> = list
-        .skip_while(|info| {
-            match (info, until) {
-                (Ok(info), Some(until)) => info.upid.starttime > until,
-                (Ok(_), None) => false,
-                (Err(_), _) => false,
-            }
-        })
-        .take_while(|info| {
-            match (info, since) {
-                (Ok(info), Some(since)) => info.upid.starttime > since,
-                (Ok(_), None) => true,
-                (Err(_), _) => false,
-            }
-        })
-        .filter_map(|info| {
+    let mut skipped = 0;
+    let mut result: Vec<TaskListItem> = Vec::new();
+
+    for info in list {
         let info = match info {
             Ok(info) => info,
-            Err(_) => return None,
+            Err(_) => break,
         };
 
+        if let Some(until) = until {
+            if info.upid.starttime > until {
+                continue;
+            }
+        }
+
+        if let Some(since) = since {
+            if let Some(ref state) = info.state {
+                if state.endtime() < since {
+                    // we reached the tasks that ended before our 'since'
+                    // so we can stop iterating
+                    break;
+                }
+            }
+            if info.upid.starttime < since {
+                continue;
+            }
+        }
+
         if !list_all && check_task_access(&auth_id, &info.upid).is_err() {
-            return None;
+            continue;
         }
 
         if let Some(needle) = &userfilter {
-            if !info.upid.auth_id.to_string().contains(needle) { return None; }
+            if !info.upid.auth_id.to_string().contains(needle) { continue; }
         }
 
         if let Some(store) = store {
-            if !check_job_store(&info.upid, store) {
-                return None;
-            }
+            if !check_job_store(&info.upid, store) { continue; }
         }
 
         if let Some(typefilter) = &typefilter {
-            if !info.upid.worker_type.contains(typefilter) {
-                return None;
-            }
+            if !info.upid.worker_type.contains(typefilter) { continue; }
         }
 
         match (&info.state, &statusfilter) {
-            (Some(_), _) if running => return None,
-            (Some(crate::server::TaskState::OK { .. }), _) if errors => return None,
+            (Some(_), _) if running => continue,
+            (Some(crate::server::TaskState::OK { .. }), _) if errors => continue,
             (Some(state), Some(filters)) => {
                 if !filters.contains(&state.tasktype()) {
-                    return None;
+                    continue;
                 }
             },
-            (None, Some(_)) => return None,
+            (None, Some(_)) => continue,
             _ => {},
         }
 
-        Some(info.into())
-    }).skip(start as usize)
-        .take(limit)
-        .collect();
+        if skipped < start as usize {
+            skipped += 1;
+            continue;
+        }
+
+        result.push(info.into());
+
+        if result.len() >= limit {
+            break;
+        }
+    }
 
     let mut count = result.len() + start as usize;
     if !result.is_empty() && result.len() >= limit { // we have a 'virtual' entry as long as we have any new
-- 
2.30.2