Re: [pbs-devel] [PATCH proxmox-backup] SnapshotVerifyState: use enum for state

[Dietmar Maurer <dietmar@proxmox.com>]

Why not a bool ?

> On 09/15/2020 10:19 AM Stefan Reiter <s.reiter@proxmox.com> wrote:
> 
>  
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/api2/backup.rs             |  8 +++-----
>  src/api2/backup/environment.rs |  4 ++--
>  src/api2/types/mod.rs          | 17 ++++++++++++++---
>  src/backup/verify.rs           |  7 +++----
>  4 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/src/api2/backup.rs b/src/api2/backup.rs
> index 371260dc..66854d4c 100644
> --- a/src/api2/backup.rs
> +++ b/src/api2/backup.rs
> @@ -120,11 +120,9 @@ async move {
>              let verify = manifest.unprotected["verify_state"].clone();
>              match serde_json::from_value::<SnapshotVerifyState>(verify) {
>                  Ok(verify) => {
> -                    if verify.state != "ok" {
> -                        // verify failed, treat as if no previous backup exists
> -                        None
> -                    } else {
> -                        Some(info)
> +                    match verify.state {
> +                        VerifyState::Ok => Some(info),
> +                        VerifyState::Failed => None,
>                      }
>                  },
>                  Err(_) => {
> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
> index 22b96c22..d515bf30 100644
> --- a/src/api2/backup/environment.rs
> +++ b/src/api2/backup/environment.rs
> @@ -9,7 +9,7 @@ use proxmox::tools::digest_to_hex;
>  use proxmox::tools::fs::{replace_file, CreateOptions};
>  use proxmox::api::{RpcEnvironment, RpcEnvironmentType};
>  
> -use crate::api2::types::{Userid, SnapshotVerifyState};
> +use crate::api2::types::{Userid, SnapshotVerifyState, VerifyState};
>  use crate::backup::*;
>  use crate::server::WorkerTask;
>  use crate::server::formatter::*;
> @@ -466,7 +466,7 @@ impl BackupEnvironment {
>                  let mark_msg = if let Some(ref last_backup) = self.last_backup {
>                      let last_dir = &last_backup.backup_dir;
>                      let verify_state = SnapshotVerifyState {
> -                        state: "failed".to_owned(),
> +                        state: VerifyState::Failed,
>                          upid: self.worker.upid().clone(),
>                      };
>  
> diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
> index e29a7e37..5495d693 100644
> --- a/src/api2/types/mod.rs
> +++ b/src/api2/types/mod.rs
> @@ -380,13 +380,24 @@ pub struct GroupListItem {
>      pub owner: Option<Userid>,
>  }
>  
> +#[api()]
> +#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)]
> +#[serde(rename_all = "lowercase")]
> +/// Result of a verify operation.
> +pub enum VerifyState {
> +    /// Verification was successful
> +    Ok,
> +    /// Verification reported one or more errors
> +    Failed,
> +}
> +
>  #[api(
>      properties: {
>          upid: {
>              schema: UPID_SCHEMA
>          },
>          state: {
> -            type: String
> +            type: VerifyState
>          },
>      },
>  )]
> @@ -395,8 +406,8 @@ pub struct GroupListItem {
>  pub struct SnapshotVerifyState {
>      /// UPID of the verify task
>      pub upid: UPID,
> -    /// State of the verification. "failed" or "ok"
> -    pub state: String,
> +    /// State of the verification. Enum.
> +    pub state: VerifyState,
>  }
>  
>  #[api(
> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
> index b75cdfb8..1fad6187 100644
> --- a/src/backup/verify.rs
> +++ b/src/backup/verify.rs
> @@ -283,7 +283,7 @@ pub fn verify_backup_dir(
>  
>      let mut error_count = 0;
>  
> -    let mut verify_result = "ok";
> +    let mut verify_result = VerifyState::Ok;
>      for info in manifest.files() {
>          let result = proxmox::try_block!({
>              worker.log(format!("  check {}", info.filename));
> @@ -316,20 +316,19 @@ pub fn verify_backup_dir(
>          if let Err(err) = result {
>              worker.log(format!("verify {}:{}/{} failed: {}", datastore.name(), backup_dir, info.filename, err));
>              error_count += 1;
> -            verify_result = "failed";
> +            verify_result = VerifyState::Failed;
>          }
>  
>      }
>  
>      let verify_state = SnapshotVerifyState {
> -        state: verify_result.to_string(),
> +        state: verify_result,
>          upid: worker.upid().clone(),
>      };
>      manifest.unprotected["verify_state"] = serde_json::to_value(verify_state)?;
>      datastore.store_manifest(&backup_dir, serde_json::to_value(manifest)?)
>          .map_err(|err| format_err!("unable to store manifest blob - {}", err))?;
>  
> -
>      Ok(error_count == 0)
>  }
>  
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel