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

[Thomas Lamprecht]

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,
};

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

> +        }
> +    }
> +
> +    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);

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

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