[pbs-devel] [PATCH proxmox-backup v2 1/9] server: change status of a task from a string to an enum

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

representing a state via an enum makes more sense in this case
we also implement FromStr and Display to make it easy to convet from/to
a string

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v2
 src/api2/admin/sync.rs    |  2 +-
 src/api2/node/tasks.rs    |  8 ++--
 src/api2/types/mod.rs     |  2 +-
 src/server/worker_task.rs | 88 ++++++++++++++++++++++++++++++---------
 4 files changed, 74 insertions(+), 26 deletions(-)

diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
index f06625f6..79af03bc 100644
--- a/src/api2/admin/sync.rs
+++ b/src/api2/admin/sync.rs
@@ -56,7 +56,7 @@ pub fn list_sync_jobs(
         if let Some(task) = last_tasks.get(&job.id) {
             job.last_run_upid = Some(task.upid_str.clone());
             if let Some((endtime, status)) = &task.state {
-                job.last_run_state = Some(String::from(status));
+                job.last_run_state = Some(status.to_string());
                 job.last_run_endtime = Some(*endtime);
                 last = *endtime;
             }
diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index 6917869e..0bb043d9 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -10,7 +10,7 @@ use proxmox::{identity, list_subdirs_api_method, sortable};
 
 use crate::tools;
 use crate::api2::types::*;
-use crate::server::{self, UPID};
+use crate::server::{self, UPID, TaskState};
 use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
 use crate::config::cached_user_info::CachedUserInfo;
 
@@ -105,9 +105,9 @@ async fn get_task_status(
     if crate::server::worker_is_active(&upid).await? {
         result["status"] = Value::from("running");
     } else {
-        let exitstatus = crate::server::upid_read_status(&upid).unwrap_or(String::from("unknown"));
+        let exitstatus = crate::server::upid_read_status(&upid).unwrap_or(TaskState::Unknown);
         result["status"] = Value::from("stopped");
-        result["exitstatus"] = Value::from(exitstatus);
+        result["exitstatus"] = Value::from(exitstatus.to_string());
     };
 
     Ok(result)
@@ -352,7 +352,7 @@ pub fn list_tasks(
 
         if let Some(ref state) = info.state {
             if running { continue; }
-            if errors && state.1 == "OK" {
+            if errors && state.1 == crate::server::TaskState::OK {
                 continue;
             }
         }
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index fb386d46..a619810d 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -595,7 +595,7 @@ impl From<crate::server::TaskListInfo> for TaskListItem {
     fn from(info: crate::server::TaskListInfo) -> Self {
         let (endtime, status) = info
             .state
-            .map_or_else(|| (None, None), |(a,b)| (Some(a), Some(b)));
+            .map_or_else(|| (None, None), |(a,b)| (Some(a), Some(b.to_string())));
 
         TaskListItem {
             upid: info.upid_str,
diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs
index 852f3ecc..876666d6 100644
--- a/src/server/worker_task.rs
+++ b/src/server/worker_task.rs
@@ -11,6 +11,7 @@ use futures::*;
 use lazy_static::lazy_static;
 use nix::unistd::Pid;
 use serde_json::{json, Value};
+use serde::{Serialize, Deserialize};
 use tokio::sync::oneshot;
 
 use proxmox::sys::linux::procfs;
@@ -190,8 +191,8 @@ pub fn create_task_log_dirs() -> Result<(), Error> {
 }
 
 /// Read exits status from task log file
-pub fn upid_read_status(upid: &UPID) -> Result<String, Error> {
-    let mut status = String::from("unknown");
+pub fn upid_read_status(upid: &UPID) -> Result<TaskState, Error> {
+    let mut status = TaskState::Unknown;
 
     let path = upid.log_path();
 
@@ -212,12 +213,8 @@ pub fn upid_read_status(upid: &UPID) -> Result<String, Error> {
         match iter.next() {
             None => continue,
             Some(rest) => {
-                if rest == "OK" {
-                    status = String::from(rest);
-                } else if rest.starts_with("WARNINGS: ") {
-                    status = String::from(rest);
-                } else if rest.starts_with("ERROR: ") {
-                    status = String::from(&rest[7..]);
+                if let Ok(state) = rest.parse() {
+                    status = state;
                 }
             }
         }
@@ -226,6 +223,59 @@ pub fn upid_read_status(upid: &UPID) -> Result<String, Error> {
     Ok(status)
 }
 
+/// Task State
+#[derive(Debug, PartialEq, Serialize, Deserialize)]
+pub enum TaskState {
+    /// The Task ended with an undefined state
+    Unknown,
+    /// The Task ended and there were no errors or warnings
+    OK,
+    /// The Task had 'count' amount of warnings and no errors
+    Warning { count: u64 },
+    /// The Task ended with the error described in 'message'
+    Error { message: String },
+}
+
+impl TaskState {
+    fn result_text(&self) -> String {
+        match self {
+            TaskState::Error { message } => format!("TASK ERROR: {}", message),
+            other => format!("TASK {}", other),
+        }
+    }
+}
+
+impl std::fmt::Display for TaskState {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            TaskState::Unknown => write!(f, "unknown"),
+            TaskState::OK => write!(f, "OK"),
+            TaskState::Warning { count } => write!(f, "WARNINGS: {}", count),
+            TaskState::Error { message } => write!(f, "{}", message),
+        }
+    }
+}
+
+impl std::str::FromStr for TaskState {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        if s == "unknown" {
+            Ok(TaskState::Unknown)
+        } else if s == "OK" {
+            Ok(TaskState::OK)
+        } else if s.starts_with("WARNINGS: ") {
+            let count: u64 = s[10..].parse()?;
+            Ok(TaskState::Warning{ count })
+        } else if s.len() > 0 {
+            let message = if s.starts_with("ERROR: ") { &s[7..] } else { s }.to_string();
+            Ok(TaskState::Error{ message })
+        } else {
+            bail!("unable to parse Task Status '{}'", s);
+        }
+    }
+}
+
 /// Task details including parsed UPID
 ///
 /// If there is no `state`, the task is still running.
@@ -236,9 +286,7 @@ pub struct TaskListInfo {
     /// UPID string representation
     pub upid_str: String,
     /// Task `(endtime, status)` if already finished
-    ///
-    /// The `status` is either `unknown`, `OK`, `WARN`, or `ERROR: ...`
-    pub state: Option<(i64, String)>, // endtime, status
+    pub state: Option<(i64, TaskState)>, // endtime, status
 }
 
 // atomically read/update the task list, update status of finished tasks
@@ -278,14 +326,14 @@ fn update_active_workers(new_upid: Option<&UPID>) -> Result<Vec<TaskListInfo>, E
                     None => {
                         println!("Detected stopped UPID {}", upid_str);
                         let status = upid_read_status(&upid)
-                            .unwrap_or_else(|_| String::from("unknown"));
+                            .unwrap_or_else(|_| TaskState::Unknown);
                         finish_list.push(TaskListInfo {
                             upid, upid_str, state: Some((Local::now().timestamp(), status))
                         });
                     },
                     Some((endtime, status)) => {
                         finish_list.push(TaskListInfo {
-                            upid, upid_str, state: Some((endtime, status))
+                            upid, upid_str, state: Some((endtime, status.parse()?))
                         })
                     }
                 }
@@ -503,23 +551,23 @@ impl WorkerTask {
         Ok(upid_str)
     }
 
-    /// get the Text of the result
-    pub fn get_log_text(&self, result: &Result<(), Error>) -> String {
-
+    /// create state from self and a result
+    pub fn create_state(&self, result: &Result<(), Error>) -> TaskState {
         let warn_count = self.data.lock().unwrap().warn_count;
 
         if let Err(err) = result {
-            format!("ERROR: {}", err)
+            TaskState::Error { message: err.to_string() }
         } else if warn_count > 0 {
-            format!("WARNINGS: {}", warn_count)
+            TaskState::Warning { count: warn_count }
         } else {
-            "OK".to_string()
+            TaskState::OK
         }
     }
 
     /// Log task result, remove task from running list
     pub fn log_result(&self, result: &Result<(), Error>) {
-        self.log(format!("TASK {}", self.get_log_text(result)));
+        let state = self.create_state(result);
+        self.log(state.result_text());
 
         WORKER_TASK_LIST.lock().unwrap().remove(&self.upid.task_id);
         let _ = update_active_workers(None);
-- 
2.20.1