Re: [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize

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

On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
[..]
> diff --git a/proxinstall b/proxinstall
> index 904668e..84f1a91 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -1488,7 +1488,7 @@ sub create_hdoption_view {
>
>      my $tmp;
>
> -    if (($tmp = &$get_float($spinbutton_hdsize)) && ($tmp != $hdsize)) {
> +    if (defined($tmp = &$get_float($spinbutton_hdsize))) {
>          Proxmox::Install::Config::set_hdsize($tmp);
>      } else {
>          Proxmox::Install::Config::set_hdsize(undef);
> @@ -1607,9 +1607,11 @@ sub create_hdsel_view {
>                  $target_hds = [map { $_->[1] } @$devlist];
>              } else {
>                  my $target_hd = Proxmox::Install::Config::get_target_hd();
> +                my $hdsize = Proxmox::Install::Config::get_hdsize();
>                  eval {
>                      my $target_block_size = Proxmox::Sys::Block::logical_blocksize($target_hd);
>                      Proxmox::Install::legacy_bios_4k_check($target_block_size);
> +                    Proxmox::Install::swapsize_check($hdsize);
>                  };
>                  if (my $err = $@) {
>                      Proxmox::UI::message("Warning: $err\n");

Very much a high-level nit; but: Could we maybe use the chance and unify
the error messages between GUI & TUI here?

E.g. in the TUI its

  "ext4: Swap size x GiB cannot .."

vs. in the GUI:

  "Warning: swap size x GiB cannot .."

Especially since the above (including the 4Kn check) are not warnings
but hard errors, i.e. not letting the user continue the installation.
So stating it as a warning does not really make sense in the GUI.

> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
> index af119e2..f29e1d4 100644
> --- a/proxmox-auto-installer/src/utils.rs
> +++ b/proxmox-auto-installer/src/utils.rs
> @@ -12,6 +12,7 @@ use crate::{
>  };
>  use proxmox_installer_common::{
>      ROOT_PASSWORD_MIN_LENGTH,
> +    disk_checks::check_swapsize,
>      options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption, email_validate},
>      setup::{
>          InstallBtrfsOption, InstallConfig, InstallFirstBootSetup, InstallRootPassword,
> @@ -397,6 +398,15 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
>              );
>          }
>      }
> +
> +    if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
> +        if let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize) {
> +            if let Err(err) = check_swapsize(swapsize, hdsize) {
> +                bail!(err);
> +            }

How about just

  check_swapsize(swapsize, hdsize)?

here?

(See also below w.r.t. anyhow)

> +        }
> +    }
> +
>      Ok(())
>  }
[..]
> diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
> index d535837..37a791f 100644
> --- a/proxmox-installer-common/src/disk_checks.rs
> +++ b/proxmox-installer-common/src/disk_checks.rs
[..]
> @@ -49,6 +49,36 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
>      Ok(())
>  }
>
> +/// Checks whether the configured swap size exceeds the allowed threshold.
> +///
> +/// # Arguments
> +///
> +/// * `swapsize` - The size of the swap in GiB
> +/// * `hdsize` - The total size of the hard disk in GiB
> +pub fn check_swapsize(swapsize: f64, hdsize: f64) -> Result<(), String> {
> +    let threshold = hdsize / 8.0;
> +    if swapsize > threshold {
> +        return Err(format!(
> +            "Swap size {swapsize} GiB cannot be greater than {threshold} GiB (hard disk size / 8)",
> +        ));
> +    }
> +    Ok(())
> +}

How about using an `anyhow::Result` here? Then it could just be a
bail!("Swap size .."), or using ensure!()

As for proxmox-tui-installer; we already pull in the anyhow crate
transitively there through proxmox-installer-common. Long-term we want
to use it there too for new code / refactorings anyway, so that would be
fine IMO.

[..]
> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
> index e87b040..b94cf38 100644
> --- a/proxmox-tui-installer/src/views/bootdisk.rs
> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
> @@ -17,7 +17,9 @@ use crate::InstallerState;
>  use crate::options::FS_TYPES;
>
>  use proxmox_installer_common::{
> -    disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
> +    disk_checks::{
> +        check_disks_4kn_legacy_boot, check_for_duplicate_disks, check_lvm_bootdisk_opts,
> +    },
>      options::{
>          AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
>          Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
> @@ -261,6 +263,8 @@ impl AdvancedBootdiskOptionsView {
>                  .get_values()
>                  .ok_or("Failed to retrieve advanced bootdisk options")?;
>
> +            check_lvm_bootdisk_opts(&advanced).map_err(|err| format!("{fstype}: {err}"))?;

With anyhow, this then also can use `.context(fstype.to_string())`
instead.

> +
>              Ok(BootdiskOptions {
>                  disks: vec![disk],
>                  fstype,



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel