all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Christian Ebner <c.ebner@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH v2 proxmox-backup 4/6] fix #6072: server: sync encrypted or verified snapshots only
Date: Wed, 2 Apr 2025 15:57:06 +0200	[thread overview]
Message-ID: <3de5cc1a-f27b-4ae8-bdd5-544782c5d5a0@proxmox.com> (raw)
In-Reply-To: <850492d0-6e93-48f0-9bd0-69ec47e4d1e0@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


  reply	other threads:[~2025-04-02 13:57 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-18 11:39 [pbs-devel] [PATCH v2 proxmox proxmox-backup 0/6] fix #6072: sync encrypted/verified " Christian Ebner
2025-03-18 11:39 ` [pbs-devel] [PATCH v2 proxmox 1/6] pbs-api-types: sync: add sync encrypted/verified snapshots only flags Christian Ebner
2025-03-18 11:39 ` [pbs-devel] [PATCH v2 proxmox-backup 2/6] server: pull: refactor snapshot pull logic Christian Ebner
2025-04-02 13:54   ` [pbs-devel] applied: " Thomas Lamprecht
2025-03-18 11:39 ` [pbs-devel] [PATCH v2 proxmox-backup 3/6] api: sync: honor sync jobs encrypted/verified only flags Christian Ebner
2025-03-18 11:39 ` [pbs-devel] [PATCH v2 proxmox-backup 4/6] fix #6072: server: sync encrypted or verified snapshots only Christian Ebner
2025-04-02 13:29   ` Thomas Lamprecht
2025-04-02 13:57     ` Christian Ebner [this message]
2025-04-02 14:11       ` Thomas Lamprecht
2025-03-18 11:39 ` [pbs-devel] [PATCH v2 proxmox-backup 5/6] bin: manager: expose encrypted/verified only flags for cli Christian Ebner
2025-03-18 11:39 ` [pbs-devel] [PATCH v2 proxmox-backup 6/6] www: expose encrypted/verified only flags in the sync job edit Christian Ebner
2025-04-02 15:30 ` [pbs-devel] superseded: [PATCH v2 proxmox proxmox-backup 0/6] fix #6072: sync encrypted/verified snapshots only Christian Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3de5cc1a-f27b-4ae8-bdd5-544782c5d5a0@proxmox.com \
    --to=c.ebner@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal