Re: [pbs-devel] [PATCH v2 proxmox-backup 4/6] fix #6072: server: sync encrypted or verified snapshots only

[Christian Ebner <c.ebner@proxmox.com>]

On 4/2/25 15:29, Thomas Lamprecht wrote:
> Am 18.03.25 um 12:39 schrieb Christian Ebner:
>> @@ -402,6 +403,55 @@ async fn pull_snapshot<'a>(
>>   
>>       let manifest = BackupManifest::try_from(tmp_manifest_blob)?;
>>   
>> +    if params.verified_only {
>> +        let mut snapshot_verified = false;
>> +        if let Ok(Some(verify_state)) = manifest.verify_state() {
>> +            if let VerifyState::Ok = verify_state.state {
>> +                snapshot_verified = true;
>> +            }
>> +        }
> 
> nit: IMO this would read slightly nicer as match, but no hard feelings.
> E.g. like (untested):
> 
> let snapshot_verified = match source_manifest.verify_state() {
>      Ok(Some(VerifyState::Ok)) => true,
>      _ => false,
> };

While that reads much nicer, destructuring does not work as the actual 
verify state is stored within the `verify_state.state`. The alternative 
would be to use a match guard, but that might be more confusing?

> 
>> +
>> +        if !snapshot_verified {
>> +            info!(
>> +                "Snapshot {} not verified but verified-only set, snapshot skipped",
>> +                snapshot.dir(),
>> +            );
>> +            if is_new {
>> +                let path = snapshot.full_path();
>> +                // safe to remove as locked by caller
>> +                std::fs::remove_dir_all(&path).map_err(|err| {
>> +                    format_err!("removing temporary backup snapshot {path:?} failed - {err}")
>> +                })?;
>> +            }
>> +            return Ok(sync_stats);
> 
> Maybe it might be nicer to use an ignore_snapshot bool shared by this and the
> encrypted-only branch and then move the early exit after that to a common if?

Acked, will adapt that according to your suggestion.

> 
>> +        }
>> +    }
>> +
>> +    if params.encrypted_only {
>> +        let mut snapshot_encrypted = true;
>> +        // Consider only encrypted if all files in the manifest are marked as encrypted
>> +        for file in manifest.files() {
>> +            if file.chunk_crypt_mode() != CryptMode::Encrypt {
>> +                snapshot_encrypted = false;
> 
> could use break after this, the value of snapshot_encrypted won't change after
> this anymore.
> 
>> +            }
>> +        }
> 
> Alternatively use a more functional style, e.g. something like (untested):
> 
> let snapshot_encrypted = source_manifest
>      .files()
>      .all(|&file| file.chunk_crypt_mode() == CryptMode::Encrypt);

This is more readable IMO, so will adapt to that!

> 
>> +
>> +        if !snapshot_encrypted {
>> +            info!(
>> +                "Snapshot {} not encrypted but encrypted-only set, snapshot skipped",
>> +                snapshot.dir(),
>> +            );
>> +            if is_new {
>> +                let path = snapshot.full_path();
>> +                // safe to remove as locked by caller
>> +                std::fs::remove_dir_all(&path).map_err(|err| {
>> +                    format_err!("removing temporary backup snapshot {path:?} failed - {err}")
>> +                })?;
>> +            }
>> +            return Ok(sync_stats);
>> +        }
>> +    }
>> +
>>       for item in manifest.files() {
>>           let mut path = snapshot.full_path();
>>           path.push(&item.filename);
> 
> 
>>   use pbs_client::{BackupRepository, BackupWriter, HttpClient, MergedChunkInfo, UploadOptions};
>>   use pbs_config::CachedUserInfo;
>> @@ -810,6 +811,35 @@ pub(crate) async fn push_snapshot(
>>           }
>>       };
>>   
>> +    if params.verified_only {
>> +        let mut snapshot_verified = false;
>> +        if let Ok(Some(verify_state)) = source_manifest.verify_state() {
>> +            if let VerifyState::Ok = verify_state.state {
>> +                snapshot_verified = true;
>> +            }
>> +        }
> 
> same as above w.r.t. code style nit.

Unfortunately same as above.

> 
>> +
>> +        if !snapshot_verified {
>> +            info!("Snapshot {snapshot} not verified but verified-only set, snapshot skipped");
>> +            return Ok(stats);
>> +        }
>> +    }
>> +
>> +    if params.encrypted_only {
>> +        let mut snapshot_encrypted = true;
>> +        // Consider only encrypted if all files in the manifest are marked as encrypted
>> +        for file in source_manifest.files() {
>> +            if file.chunk_crypt_mode() != CryptMode::Encrypt {
>> +                snapshot_encrypted = false;
> 
> same as above w.r.t. code style nit.
Acked, will adapt this.



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel