* [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into`
@ 2026-04-27 7:55 Dominik Csapak
2026-04-27 8:19 ` Fabian Grünbichler
0 siblings, 1 reply; 3+ messages in thread
From: Dominik Csapak @ 2026-04-27 7:55 UTC (permalink / raw)
To: pbs-devel
there was still a single log message there with a regular 'info!' macro
that didn't use the LogLineSender.
Fix that by requiring the LogLineSender there too.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
src/server/pull.rs | 8 ++++++--
src/server/sync.rs | 42 +++++++++++++++++++++++++++++++++---------
2 files changed, 39 insertions(+), 11 deletions(-)
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?;
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())?;
@@ -654,7 +654,11 @@ async fn pull_snapshot<'a>(
let mut tmp_manifest_name = manifest_name.clone();
tmp_manifest_name.set_extension("tmp");
let Some(tmp_manifest_blob) = 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),
+ )
.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 into.
- async fn load_file_into(&self, filename: &str, into: &Path) -> Result<Option<DataBlob>, Error>;
+ async fn load_file_into(
+ &self,
+ filename: &str,
+ into: &Path,
+ log_sender: Arc<LogLineSender>,
+ ) -> Result<Option<DataBlob>, Error>;
/// 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))
}
- async fn load_file_into(&self, filename: &str, into: &Path) -> Result<Option<DataBlob>, Error> {
+ async fn load_file_into(
+ &self,
+ filename: &str,
+ into: &Path,
+ log_sender: Arc<LogLineSender>,
+ ) -> Result<Option<DataBlob>, Error> {
let mut tmp_file = std::fs::OpenOptions::new()
.write(true)
.create(true)
@@ -158,10 +168,15 @@ impl SyncSourceReader for RemoteSourceReader {
match err.downcast_ref::<HttpError>() {
Some(HttpError { code, message }) => match *code {
StatusCode::NOT_FOUND => {
- info!(
- "Snapshot {}: skipped because vanished since start of sync",
- &self.dir
- );
+ log_sender
+ .log(
+ Level::INFO,
+ format!(
+ "Snapshot {}: skipped because vanished since start of sync",
+ &self.dir
+ ),
+ )
+ .await?;
return Ok(None);
}
_ => {
@@ -245,7 +260,12 @@ impl SyncSourceReader for LocalSourceReader {
Ok(Arc::new(chunk_reader))
}
- async fn load_file_into(&self, filename: &str, into: &Path) -> Result<Option<DataBlob>, Error> {
+ async fn load_file_into(
+ &self,
+ filename: &str,
+ into: &Path,
+ _log_sender: Arc<LogLineSender>,
+ ) -> Result<Option<DataBlob>, Error> {
let mut tmp_file = 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(
--
2.47.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into`
2026-04-27 7:55 [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into` Dominik Csapak
@ 2026-04-27 8:19 ` Fabian Grünbichler
2026-04-27 8:50 ` Fabian Grünbichler
0 siblings, 1 reply; 3+ messages in thread
From: Fabian Grünbichler @ 2026-04-27 8:19 UTC (permalink / raw)
To: Dominik Csapak, pbs-devel
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.
>
> Fix that by requiring the LogLineSender there too.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> src/server/pull.rs | 8 ++++++--
> src/server/sync.rs | 42 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 39 insertions(+), 11 deletions(-)
>
> 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?;
>
> 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())?;
>
> @@ -654,7 +654,11 @@ async fn pull_snapshot<'a>(
> let mut tmp_manifest_name = manifest_name.clone();
> tmp_manifest_name.set_extension("tmp");
> let Some(tmp_manifest_blob) = 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 into.
> - async fn load_file_into(&self, filename: &str, into: &Path) -> Result<Option<DataBlob>, Error>;
> + async fn load_file_into(
> + &self,
> + filename: &str,
> + into: &Path,
> + log_sender: Arc<LogLineSender>,
> + ) -> Result<Option<DataBlob>, Error>;
>
> /// 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))
> }
>
> - async fn load_file_into(&self, filename: &str, into: &Path) -> Result<Option<DataBlob>, Error> {
> + async fn load_file_into(
> + &self,
> + filename: &str,
> + into: &Path,
> + log_sender: Arc<LogLineSender>,
> + ) -> Result<Option<DataBlob>, Error> {
> let mut tmp_file = std::fs::OpenOptions::new()
> .write(true)
> .create(true)
> @@ -158,10 +168,15 @@ impl SyncSourceReader for RemoteSourceReader {
> match err.downcast_ref::<HttpError>() {
> Some(HttpError { code, message }) => match *code {
> StatusCode::NOT_FOUND => {
> - info!(
> - "Snapshot {}: skipped because vanished since start of sync",
> - &self.dir
> - );
> + log_sender
> + .log(
> + Level::INFO,
> + format!(
> + "Snapshot {}: skipped because vanished 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..
> }
> _ => {
> @@ -245,7 +260,12 @@ impl SyncSourceReader for LocalSourceReader {
> Ok(Arc::new(chunk_reader))
> }
>
> - async fn load_file_into(&self, filename: &str, into: &Path) -> Result<Option<DataBlob>, Error> {
> + async fn load_file_into(
> + &self,
> + filename: &str,
> + into: &Path,
> + _log_sender: Arc<LogLineSender>,
> + ) -> Result<Option<DataBlob>, Error> {
> let mut tmp_file = 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(
> --
> 2.47.3
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into`
2026-04-27 8:19 ` Fabian Grünbichler
@ 2026-04-27 8:50 ` Fabian Grünbichler
0 siblings, 0 replies; 3+ messages in thread
From: Fabian Grünbichler @ 2026-04-27 8:50 UTC (permalink / raw)
To: Dominik Csapak, pbs-devel
On April 27, 2026 10:19 am, Fabian Grünbichler wrote:
> On April 27, 2026 9:55 am, Dominik Csapak wrote:
>> [..]
> 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..
sent: https://lore.proxmox.com/pbs-devel/20260427084952.303245-1-f.gruenbichler@proxmox.com/T/#u
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-27 8:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-27 7:55 [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into` Dominik Csapak
2026-04-27 8:19 ` Fabian Grünbichler
2026-04-27 8:50 ` Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox