[pbs-devel] [PATCH proxmox-backup v4 1/4] snapshot: add helper function to retrieve verify_state

[Fabian Grünbichler]

> Gabriel Goller <g.goller at proxmox.com> hat am 22.11.2024 10:02 CET geschrieben:
> On 21.11.2024 20:17, Fabian Grünbichler wrote:
> >> Gabriel Goller <g.goller at proxmox.com> hat am 21.11.2024 14:35 CET geschrieben:
> >> @@ -159,13 +159,12 @@ fn upgrade_to_backup_protocol(
> >>              let info = backup_group.last_backup(true).unwrap_or(None);
> >>              if let Some(info) = info {
> >>                  let (manifest, _) = info.backup_dir.load_manifest()?;
> >> -                let verify = manifest.unprotected["verify_state"].clone();
> >> -                match serde_json::from_value::<SnapshotVerifyState>(verify) {
> >> -                    Ok(verify) => match verify.state {
> >> +                match manifest.verify_state() {
> >> +                    Ok(Some(verify)) => match verify.state {
> >>                          VerifyState::Ok => Some(info),
> >>                          VerifyState::Failed => None,
> >>                      },
> >> -                    Err(_) => {
> >> +                    Ok(None) | Err(_) => {
> >>                          // no verify state found, treat as valid
> >
> >this as well, although it might make sense to log this here as well (pre-existing)
> 
> You mean separating the Ok(None) and Err(_) arm and `warn()` on the
> Err(_) one?

yes, exactly.. this should hopefully never trigger, but if it does, not logging it just causes confusion - with log output, there's a chance that additional information gives the deciding clue what's going on :)

> >>                          Some(info)
> >>                      }
> >> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
> >> index 6ef7e8eb3ebb..20c605c4dde6 100644
> >> --- a/src/backup/verify.rs
> >> +++ b/src/backup/verify.rs
> >> @@ -553,10 +553,9 @@ pub fn verify_filter(
> >>          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 manifest.verify_state() {
> >> +        Ok(None) | Err(_) => true, // no last verification, always include
> >
> >same here! I think/hope the Err path for these should only trigger when somebody messes up manifests, but..
> 
> Yep.