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 408DF1FF0AF for ; Thu, 24 Sep 2026 18:06:51 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D572F2161D; Thu, 24 Sep 2026 18:06:50 +0200 (CEST) Message-ID: Date: Thu, 24 Sep 2026 18:06:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup 1/2] backup writer: add constructor for BackupWriterOptions To: Thomas Ellmenreich , pbs-devel@lists.proxmox.com References: <20260923102759.139819-1-t.ellmenreich@proxmox.com> <20260923102759.139819-2-t.ellmenreich@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260923102759.139819-2-t.ellmenreich@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790266006808 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.633 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_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: HV2YJMCGUWOT5OBIXUZT4DIL4I6MUKIQ X-Message-ID-Hash: HV2YJMCGUWOT5OBIXUZT4DIL4I6MUKIQ X-MailFrom: c.ebner@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 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 > --- > 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 { > > 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>, > + 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 { > 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( > ¶ms.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())?;