From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0EF7889CC for ; Thu, 22 Jun 2023 12:29:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DCFB42709A for ; Thu, 22 Jun 2023 12:28:36 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 22 Jun 2023 12:28:35 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0499F42975 for ; Thu, 22 Jun 2023 12:28:35 +0200 (CEST) Date: Thu, 22 Jun 2023 12:28:33 +0200 From: Christoph Heiss To: Proxmox VE development discussion Message-ID: References: <20230622092038.205553-1-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230622092038.205553-1-s.sterz@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.077 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH installer] tui: switch to `f64` for disk sizes X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Jun 2023 10:29:07 -0000 While going over this I had a though: What about wrapping the disk size in something like: struct DiskSize(f64); and having e.g. `::from_mib()`, `.in_gib()`, `Display` impl etc.? Just a thought, but this would IMO provide some (valuable) context everywhere disk sizes are handled and hopefully avoid (more) confusion and mistakes in the future. On Thu, Jun 22, 2023 at 11:20:38AM +0200, Stefan Sterz wrote: > previously the tui used `u64` internally to represent the disk size. > since the perl-based installer expects GiB as floats and that is also > what is displayed in the tui that meant a lot of converting back and > forth. it also lead to an error where the disk sizes that were set > seemed to not have been persisted, even though the sizes were > correctly set. this commit refactors the installer to convert the size > once in the beginning and then stick to `f64`. > Pretty straight-forward changes, LGTM. Reviewed-by: Christoph Heiss Tested-by: Christoph Heiss > Signed-off-by: Stefan Sterz > --- > proxmox-tui-installer/src/options.rs | 26 ++++++++++++-------------- > proxmox-tui-installer/src/setup.rs | 16 ++++++++-------- > proxmox-tui-installer/src/views/mod.rs | 18 +++++++----------- > 3 files changed, 27 insertions(+), 33 deletions(-) > > diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs > index 5f3d295..e1218df 100644 > --- a/proxmox-tui-installer/src/options.rs > +++ b/proxmox-tui-installer/src/options.rs > @@ -86,11 +86,11 @@ pub const FS_TYPES: &[FsType] = { > > #[derive(Clone, Debug)] > pub struct LvmBootdiskOptions { > - pub total_size: u64, > - pub swap_size: Option, > - pub max_root_size: Option, > - pub max_data_size: Option, > - pub min_lvm_free: Option, > + pub total_size: f64, > + pub swap_size: Option, > + pub max_root_size: Option, > + pub max_data_size: Option, > + pub min_lvm_free: Option, > } > > impl LvmBootdiskOptions { > @@ -107,7 +107,7 @@ impl LvmBootdiskOptions { > > #[derive(Clone, Debug)] > pub struct BtrfsBootdiskOptions { > - pub disk_size: u64, > + pub disk_size: f64, > } > > impl BtrfsBootdiskOptions { > @@ -180,7 +180,7 @@ pub struct ZfsBootdiskOptions { > pub compress: ZfsCompressOption, > pub checksum: ZfsChecksumOption, > pub copies: usize, > - pub disk_size: u64, > + pub disk_size: f64, > } > > impl ZfsBootdiskOptions { > @@ -202,12 +202,12 @@ pub enum AdvancedBootdiskOptions { > Btrfs(BtrfsBootdiskOptions), > } > > -#[derive(Clone, Debug, Eq, PartialEq)] > +#[derive(Clone, Debug, PartialEq)] > pub struct Disk { > pub index: String, > pub path: String, > pub model: Option, > - pub size: u64, > + pub size: f64, > } > > impl fmt::Display for Disk { > @@ -219,11 +219,7 @@ impl fmt::Display for Disk { > // FIXME: ellipsize too-long names? > write!(f, " ({model})")?; > } > - write!( > - f, > - " ({:.2} GiB)", > - (self.size as f64) / 1024. / 1024. / 1024. > - ) > + write!(f, " ({:.2} GiB)", self.size) > } > } > > @@ -233,6 +229,8 @@ impl From<&Disk> for String { > } > } > > +impl cmp::Eq for Disk {} > + > impl cmp::PartialOrd for Disk { > fn partial_cmp(&self, other: &Self) -> Option { > self.index.partial_cmp(&other.index) > diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs > index 43e4b0d..1c5ff3e 100644 > --- a/proxmox-tui-installer/src/setup.rs > +++ b/proxmox-tui-installer/src/setup.rs > @@ -109,15 +109,15 @@ pub struct InstallConfig { > > #[serde(serialize_with = "serialize_fstype")] > filesys: FsType, > - hdsize: u64, > + hdsize: f64, > #[serde(skip_serializing_if = "Option::is_none")] > - swapsize: Option, > + swapsize: Option, > #[serde(skip_serializing_if = "Option::is_none")] > - maxroot: Option, > + maxroot: Option, > #[serde(skip_serializing_if = "Option::is_none")] > - minfree: Option, > + minfree: Option, > #[serde(skip_serializing_if = "Option::is_none")] > - maxvz: Option, > + maxvz: Option, > > #[serde(skip_serializing_if = "Option::is_none")] > zfs_opts: Option, > @@ -153,7 +153,7 @@ impl From for InstallConfig { > autoreboot: options.autoreboot as usize, > > filesys: options.bootdisk.fstype, > - hdsize: 0, > + hdsize: 0., > swapsize: None, > maxroot: None, > minfree: None, > @@ -243,13 +243,13 @@ fn deserialize_disks_map<'de, D>(deserializer: D) -> Result, D::Error> > where > D: Deserializer<'de>, > { > - let disks = >::deserialize(deserializer)?; > + let disks = >::deserialize(deserializer)?; > Ok(disks > .into_iter() > .map( > |(index, device, size_mb, model, logical_bsize, _syspath)| Disk { > index: index.to_string(), > - size: size_mb * logical_bsize, > + size: (size_mb * logical_bsize) / 1024. / 1024. / 1024., > path: device, > model: (!model.is_empty()).then_some(model), > }, > diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs > index ee96398..faa0052 100644 > --- a/proxmox-tui-installer/src/views/mod.rs > +++ b/proxmox-tui-installer/src/views/mod.rs > @@ -189,17 +189,15 @@ impl DiskSizeEditView { > } > } > > - pub fn content(mut self, content: u64) -> Self { > - let val = (content as f64) / 1024. / 1024. / 1024.; > - > + pub fn content(mut self, content: f64) -> Self { > if let Some(view) = self.view.get_child_mut(0).and_then(|v| v.downcast_mut()) { > - *view = FloatEditView::new().content(val).full_width(); > + *view = FloatEditView::new().content(content).full_width(); > } > > self > } > > - pub fn content_maybe(self, content: Option) -> Self { > + pub fn content_maybe(self, content: Option) -> Self { > if let Some(value) = content { > self.content(value) > } else { > @@ -207,20 +205,19 @@ impl DiskSizeEditView { > } > } > > - pub fn max_value(mut self, max: u64) -> Self { > + pub fn max_value(mut self, max: f64) -> Self { > if let Some(view) = self > .view > .get_child_mut(0) > .and_then(|v| v.downcast_mut::>()) > { > - let max = (max as f64) / 1024. / 1024. / 1024.; > view.get_inner_mut().set_max_value(max); > } > > self > } > > - pub fn get_content(&self) -> Option { > + pub fn get_content(&self) -> Option { > self.with_view(|v| { > v.get_child(0)? > .downcast_ref::>()? > @@ -234,7 +231,6 @@ impl DiskSizeEditView { > .flatten() > }) > .flatten() > - .map(|val| val as u64) > } > } > > @@ -285,8 +281,8 @@ where > } > } > > -impl FormViewGetValue for DiskSizeEditView { > - fn get_value(&self) -> Option { > +impl FormViewGetValue for DiskSizeEditView { > + fn get_value(&self) -> Option { > self.get_content() > } > } > -- > 2.39.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > >