Re: [PATCH proxmox-backup] sync: pull: use LogLineSender also for `load_file_into`

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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
> 
> 
> 
> 
> 
>