Re: [pve-devel] [PATCH installer] tui: switch to `f64` for disk sizes

[Christoph Heiss <c.heiss@proxmox.com>]

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.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
> Signed-off-by: Stefan Sterz <s.sterz@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
>
>