From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id AAFC51FF17C
for <inbox@lore.proxmox.com>; Wed, 2 Apr 2025 15:57:21 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 36BD51C022;
Wed, 2 Apr 2025 15:57:11 +0200 (CEST)
Message-ID: <3de5cc1a-f27b-4ae8-bdd5-544782c5d5a0@proxmox.com>
Date: Wed, 2 Apr 2025 15:57:06 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>
References: <20250318113912.335359-1-c.ebner@proxmox.com>
<20250318113912.335359-5-c.ebner@proxmox.com>
<850492d0-6e93-48f0-9bd0-69ec47e4d1e0@proxmox.com>
Content-Language: en-US, de-DE
From: Christian Ebner <c.ebner@proxmox.com>
In-Reply-To: <850492d0-6e93-48f0-9bd0-69ec47e4d1e0@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.031 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pbs-devel] [PATCH v2 proxmox-backup 4/6] fix #6072: server:
sync encrypted or verified snapshots only
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.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