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
