From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 2EC3C1FF0E6 for ; Fri, 24 Jul 2026 10:11:36 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D83F421493; Fri, 24 Jul 2026 10:11:35 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 24 Jul 2026 10:11:31 +0200 Message-Id: Subject: Re: [PATCH proxmox-backup 1/2] fix #6990: server: drop verify state on non-decrypt pull job From: "Jakob Klocker" To: "Christian Ebner" , X-Mailer: aerc 0.20.0 References: <20260709122136.352839-1-j.klocker@proxmox.com> <20260709122136.352839-2-j.klocker@proxmox.com> <901201f6-9e5d-4470-8059-3016473b0944@proxmox.com> In-Reply-To: <901201f6-9e5d-4470-8059-3016473b0944@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784880660627 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.871 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: SILEFEPWXZBU7FNE4JT6T2DV5Y2P3SSL X-Message-ID-Hash: SILEFEPWXZBU7FNE4JT6T2DV5Y2P3SSL X-MailFrom: j.klocker@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for the feedback! On Thu Jul 23, 2026 at 1:08 PM CEST, Christian Ebner wrote: > Thanks for the patch, mostly looks good to me. > > Just one comment and nit inline. > > On 7/9/26 2:22 PM, Jakob Klocker wrote: >> On a non-decrypt pull the source manifest is written to the target >> as-is, so the target inherits the source's verify_state flag instead of >> being verified independently on its own storage. Because a snapshot >> carrying a verify_state is skipped by verify jobs, the target's copy can >> never be checked. >>=20 >> The decrypt path already drops verify_state; do the same on the >> non-decrypt path when the snapshot is newly pulled or re-synced due to >> corruption. Also factor the manifest blob encoding and writing into a >> helper, shared by both paths. >>=20 >> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=3D6990 >> =20 >>[SNIP] >> +async fn write_manifest_blob( >> + manifest: &BackupManifest, >> + path: &std::path::Path, >> +) -> Result, Error> { >> + let manifest_blob =3D DataBlob::encode(manifest.to_string(None)?.as= _bytes(), None, true)?; >> + let manifest_data =3D manifest_blob.raw_data().to_vec(); >> + let mut manifest_file =3D OpenOptions::new() >> + .write(true) >> + .truncate(true) >> + .open(path) >> + .await?; >> + manifest_file.write_all(&manifest_data).await?; >> + manifest_file.flush().await?; >> + nix::unistd::fsync(manifest_file.as_raw_fd())?; > > nit: pre-existing but this is a blocking call and we are in an async=20 > context, so we should spawn a blocking tokio task for this I guess. Same= =20 > is true for the std::fs::rename which should be replaced by it's=20 > tokio::fs::rename counterpart and the call to=20 > BackupDir::cleanup_unreferenced_files(). > > Could you also include additional patches to fix these since touching thi= s? > I'll make sure to fix these in a v2. >> + Ok(manifest_data) >> +} > > comment: This should rather live as method impl on BackupManifest itself= =20 > IMO. It could further be refactored to reduce common code with=20 > pbs-datastores BackupDir::update_manifest(). > Makes sense moving this to BackupManifest. Thanks for pointing me to `update_manifest`, I'll further refactor this. >> /// Check if the decryption key should be used to decrypt the snapshot= during >> /// pull based on given pull parameter, source and optionally already = present >> /// target manifest.