From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id BD9E51FF148 for ; Mon, 27 Apr 2026 10:20:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 952AF1491B; Mon, 27 Apr 2026 10:20:31 +0200 (CEST) Date: Mon, 27 Apr 2026 10:19:53 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into` To: Dominik Csapak , pbs-devel@lists.proxmox.com References: <20260427075509.776694-1-d.csapak@proxmox.com> In-Reply-To: <20260427075509.776694-1-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1777277236.2hvl2my85r.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777277902152 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 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 Message-ID-Hash: 633HBF6F3ZJUD3PKO45WJJSFUIQ36KGP X-Message-ID-Hash: 633HBF6F3ZJUD3PKO45WJJSFUIQ36KGP X-MailFrom: f.gruenbichler@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: On April 27, 2026 9:55 am, Dominik Csapak wrote: > there was still a single log message there with a regular 'info!' macro > that didn't use the LogLineSender. >=20 > Fix that by requiring the LogLineSender there too. >=20 > Signed-off-by: Dominik Csapak > --- > src/server/pull.rs | 8 ++++++-- > src/server/sync.rs | 42 +++++++++++++++++++++++++++++++++--------- > 2 files changed, 39 insertions(+), 11 deletions(-) >=20 > diff --git a/src/server/pull.rs b/src/server/pull.rs > index 42c34732f..161b6fcb3 100644 > --- a/src/server/pull.rs > +++ b/src/server/pull.rs > @@ -448,7 +448,7 @@ async fn pull_single_archive<'a>( > .await?; > =20 > reader > - .load_file_into(archive_name, &tmp_path) > + .load_file_into(archive_name, &tmp_path, Arc::clone(&log_sender)= ) > .await > .with_context(|| archive_prefix.clone())?; > =20 > @@ -654,7 +654,11 @@ async fn pull_snapshot<'a>( > let mut tmp_manifest_name =3D manifest_name.clone(); > tmp_manifest_name.set_extension("tmp"); > let Some(tmp_manifest_blob) =3D reader > - .load_file_into(MANIFEST_BLOB_NAME.as_ref(), &tmp_manifest_name) > + .load_file_into( > + MANIFEST_BLOB_NAME.as_ref(), > + &tmp_manifest_name, > + Arc::clone(&log_sender), > + ) couldn't we handle the Ok(None) case here, and log, instead of requiring to put the log sender into the sync sources? > .await > .with_context(|| prefix.clone())? > else { > diff --git a/src/server/sync.rs b/src/server/sync.rs > index 92449ff98..2f84abb24 100644 > --- a/src/server/sync.rs > +++ b/src/server/sync.rs > @@ -106,7 +106,12 @@ pub(crate) trait SyncSourceReader: Send + Sync { > /// Asynchronously loads a file from the source into a local file. > /// `filename` is the name of the file to load from the source. > /// `into` is the path of the local file to load the source file int= o. > - async fn load_file_into(&self, filename: &str, into: &Path) -> Resul= t, Error>; > + async fn load_file_into( > + &self, > + filename: &str, > + into: &Path, > + log_sender: Arc, > + ) -> Result, Error>; > =20 > /// Tries to fetch the client log from the source and save it into a= local file. > async fn try_fetch_client_log( > @@ -146,7 +151,12 @@ impl SyncSourceReader for RemoteSourceReader { > Ok(Arc::new(chunk_reader)) > } > =20 > - async fn load_file_into(&self, filename: &str, into: &Path) -> Resul= t, Error> { > + async fn load_file_into( > + &self, > + filename: &str, > + into: &Path, > + log_sender: Arc, > + ) -> Result, Error> { > let mut tmp_file =3D std::fs::OpenOptions::new() > .write(true) > .create(true) > @@ -158,10 +168,15 @@ impl SyncSourceReader for RemoteSourceReader { > match err.downcast_ref::() { > Some(HttpError { code, message }) =3D> match *code { > StatusCode::NOT_FOUND =3D> { > - info!( > - "Snapshot {}: skipped because vanished since= start of sync", > - &self.dir > - ); > + log_sender > + .log( > + Level::INFO, > + format!( > + "Snapshot {}: skipped because vanish= ed since start of sync", > + &self.dir > + ), > + ) > + .await?; > return Ok(None); IMHO this should only return Ok(None), the `ok()` call at the end of this fn seems quite weird - if we load a file and the CRC check fails, we should not silently ignore this? this also only makes sense for the manifest, for the archives handling this correctly would make for earlier error handling.. I'll send an alternative version doing that.. > } > _ =3D> { > @@ -245,7 +260,12 @@ impl SyncSourceReader for LocalSourceReader { > Ok(Arc::new(chunk_reader)) > } > =20 > - async fn load_file_into(&self, filename: &str, into: &Path) -> Resul= t, Error> { > + async fn load_file_into( > + &self, > + filename: &str, > + into: &Path, > + _log_sender: Arc, > + ) -> Result, Error> { > let mut tmp_file =3D std::fs::OpenOptions::new() > .write(true) > .create(true) > @@ -281,8 +301,12 @@ impl SyncSourceReader for LocalSourceReader { > ) > .await?; > } else { > - self.load_file_into(CLIENT_LOG_BLOB_NAME.as_ref(), to_path) > - .await?; > + self.load_file_into( > + CLIENT_LOG_BLOB_NAME.as_ref(), > + to_path, > + Arc::clone(&log_sender), > + ) > + .await?; > } > log_sender > .log( > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20