Re: [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions

[Christian Ebner <c.ebner@proxmox.com>]

On 9/23/26 12:28 PM, Thomas Ellmenreich wrote:
> Instead of creating BackupWriterOptions directly, use a new function for it.
> The goal is to be able to remove values from the constructor and provide
> default values for them.

Might make sense to use a builder like pattern [0] here instead and set 
sensible defaults? Would especially setting the flags be more verbose 
without needing to lookup the parameter definition and order of new().

[0] https://www.lurklurk.org/effective-rust/builders.html

> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>   examples/upload-speed.rs               | 16 ++++++++--------
>   pbs-client/src/backup_writer.rs        | 22 ++++++++++++++++++++++
>   proxmox-backup-client/src/benchmark.rs | 16 ++++++++--------
>   proxmox-backup-client/src/main.rs      | 16 ++++++++--------
>   src/server/push.rs                     | 18 +++++++++---------
>   5 files changed, 55 insertions(+), 33 deletions(-)
> 
> diff --git a/examples/upload-speed.rs b/examples/upload-speed.rs
> index 5a63e0f09..a0b742854 100644
> --- a/examples/upload-speed.rs
> +++ b/examples/upload-speed.rs
> @@ -19,15 +19,15 @@ async fn upload_speed() -> Result<f64, Error> {
>   
>       let client = BackupWriter::start(
>           &client,
> -        BackupWriterOptions {
> +        BackupWriterOptions::new(
>               datastore,
> -            ns: &BackupNamespace::root(),
> -            backup: &(BackupType::Host, "speedtest".to_string(), backup_time).into(),
> -            crypt_config: None,
> -            debug: false,
> -            benchmark: true,
> -            no_cache: false,
> -        },
> +            &BackupNamespace::root(),
> +            &(BackupType::Host, "speedtest".to_string(), backup_time).into(),
> +            None,
> +            false,
> +            true,
> +            false,
> +        ),
>       )
>       .await?;
>   
> diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
> index 7f7535746..7c3cb21b7 100644
> --- a/pbs-client/src/backup_writer.rs
> +++ b/pbs-client/src/backup_writer.rs
> @@ -100,6 +100,28 @@ pub struct BackupWriterOptions<'a> {
>       pub no_cache: bool,
>   }
>   
> +impl<'a> BackupWriterOptions<'a> {
> +    pub fn new(
> +        datastore: &'a str,
> +        ns: &'a BackupNamespace,
> +        backup: &'a BackupDir,
> +        crypt_config: Option<Arc<CryptConfig>>,
> +        debug: bool,
> +        benchmark: bool,
> +        no_cache: bool,
> +    ) -> Self {
> +        Self {
> +            datastore,
> +            ns,
> +            backup,
> +            crypt_config,
> +            debug,
> +            benchmark,
> +            no_cache,
> +        }
> +    }
> +}
> +
>   impl BackupWriter {
>       fn new(h2: H2Client, abort: AbortHandle, crypt_config: Option<Arc<CryptConfig>>) -> Arc<Self> {
>           Arc::new(Self {
> diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs
> index a937bacb9..af9113ecb 100644
> --- a/proxmox-backup-client/src/benchmark.rs
> +++ b/proxmox-backup-client/src/benchmark.rs
> @@ -238,15 +238,15 @@ async fn test_upload_speed(
>       log::debug!("Connecting to backup server");
>       let client = BackupWriter::start(
>           &client,
> -        BackupWriterOptions {
> -            datastore: repo.store(),
> -            ns: &BackupNamespace::root(),
> -            backup: &(BackupType::Host, "benchmark".to_string(), backup_time).into(),
> -            crypt_config: crypt_config.clone(),
> -            debug: false,
> -            benchmark: true,
> +        BackupWriterOptions::new(
> +            repo.store(),
> +            &BackupNamespace::root(),
> +            &(BackupType::Host, "benchmark".to_string(), backup_time).into(),
> +            crypt_config.clone(),
> +            false,
> +            true,
>               no_cache,
> -        },
> +        ),
>       )
>       .await?;
>   
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 5238a75ea..3aaaac39e 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -1046,15 +1046,15 @@ async fn create_backup(
>   
>       let client = BackupWriter::start(
>           &http_client,
> -        BackupWriterOptions {
> -            datastore: repo.store(),
> -            ns: &backup_ns,
> -            backup: &snapshot,
> -            crypt_config: crypt_config.clone(),
> -            debug: true,
> -            benchmark: false,
> +        BackupWriterOptions::new(
> +            repo.store(),
> +            &backup_ns,
> +            &snapshot,
> +            crypt_config.clone(),
> +            true,
> +            false,
>               no_cache,
> -        },
> +        ),
>       )
>       .await?;
>   
> diff --git a/src/server/push.rs b/src/server/push.rs
> index 487fd91e3..665c77a8d 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -1132,17 +1132,17 @@ pub(crate) async fn push_snapshot(
>       // Writer instance locks the snapshot on the remote side
>       let backup_writer = BackupWriter::start(
>           &params.target.client,
> -        BackupWriterOptions {
> -            datastore: params.target.repo.store(),
> -            ns: &target_ns,
> -            backup: snapshot,
> -            crypt_config: encrypt_using_key
> +        BackupWriterOptions::new(
> +            params.target.repo.store(),
> +            &target_ns,
> +            snapshot,
> +            encrypt_using_key
>                   .as_ref()
>                   .map(|(_id, conf)| Arc::clone(conf)),
> -            debug: false,
> -            benchmark: false,
> -            no_cache: false,
> -        },
> +            false,
> +            false,
> +            false,
> +        ),
>       )
>       .await
>       .with_context(|| prefix.to_string())?;