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

[Gabriel Goller]

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:
>> +
>> +    /// Load the verify state from the manifest.
>> +    pub fn verify_state(&self) -> Result<Option<VerifyState>, anyhow::Error> {
>> +        let manifest = self.load_manifest()?;
>> +        Ok(manifest
>> +            .0
>> +            .verify_state()
>> +            .ok()
>> +            .flatten()
>> +            .map(|svs| svs.state))
>
>this still looks slightly wrong to me - if verify_state() returns an error, it's mapped to None (by the call to `ok()`), which would hide an inner parse error for the verification state?
>
>I think the following should be correctly bubble up errors when loading the manifest or when parsing the contained verify state while returning Ok(None) if no state is contained in the manifest:
>
>Ok(self.load_manifest()?.0.verify_state()?.map(|svs| svs.state))

I agree. I was somehow fixed on the load_manifest error always being the
outer one, but tbh it doesn't matter.

>
>> +    }
>>  }
>>
>> @@ -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?

>>                          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.