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 <c.he...@proxmox.com>
Tested-by: Christoph Heiss <c.he...@proxmox.com>
> Signed-off-by: Stefan Sterz <s.st...@proxmox.com>
> ---
> 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<u64>,
> - pub max_root_size: Option<u64>,
> - pub max_data_size: Option<u64>,
> - pub min_lvm_free: Option<u64>,
> + pub total_size: f64,
> + pub swap_size: Option<f64>,
> + pub max_root_size: Option<f64>,
> + pub max_data_size: Option<f64>,
> + pub min_lvm_free: Option<f64>,
> }
>
> 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<String>,
> - 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<cmp::Ordering> {
> 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<u64>,
> + swapsize: Option<f64>,
> #[serde(skip_serializing_if = "Option::is_none")]
> - maxroot: Option<u64>,
> + maxroot: Option<f64>,
> #[serde(skip_serializing_if = "Option::is_none")]
> - minfree: Option<u64>,
> + minfree: Option<f64>,
> #[serde(skip_serializing_if = "Option::is_none")]
> - maxvz: Option<u64>,
> + maxvz: Option<f64>,
>
> #[serde(skip_serializing_if = "Option::is_none")]
> zfs_opts: Option<InstallZfsOption>,
> @@ -153,7 +153,7 @@ impl From<InstallerOptions> 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<Vec<Disk>, D::Error>
> where
> D: Deserializer<'de>,
> {
> - let disks = <Vec<(usize, String, u64, String, u64,
> String)>>::deserialize(deserializer)?;
> + let disks = <Vec<(usize, String, f64, String, f64,
> String)>>::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<u64>) -> Self {
> + pub fn content_maybe(self, content: Option<f64>) -> 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::<ResizedView<FloatEditView>>())
> {
> - let max = (max as f64) / 1024. / 1024. / 1024.;
> view.get_inner_mut().set_max_value(max);
> }
>
> self
> }
>
> - pub fn get_content(&self) -> Option<u64> {
> + pub fn get_content(&self) -> Option<f64> {
> self.with_view(|v| {
> v.get_child(0)?
> .downcast_ref::<ResizedView<FloatEditView>>()?
> @@ -234,7 +231,6 @@ impl DiskSizeEditView {
> .flatten()
> })
> .flatten()
> - .map(|val| val as u64)
> }
> }
>
> @@ -285,8 +281,8 @@ where
> }
> }
>
> -impl FormViewGetValue<u64> for DiskSizeEditView {
> - fn get_value(&self) -> Option<u64> {
> +impl FormViewGetValue<f64> for DiskSizeEditView {
> + fn get_value(&self) -> Option<f64> {
> 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
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
