* [pve-devel] [PATCH pve-installer v3 1/7] auto: add early answer file sanity check for RAID configurations
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations Michael Köppl
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
The GUI and TUI installers already implement checks to ensure systems
have the minimum required number of disks available for the various RAID
configurations (min 2 disks for RAID1, min 4 disks for RAID10, etc).
This change adds an early check of the answer file to the
auto-installer, improving the user experience by avoiding failure during
the actual installation.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-auto-install-assistant/src/main.rs | 3 +-
proxmox-auto-installer/src/utils.rs | 18 ++++++++++-
proxmox-auto-installer/tests/parse-answer.rs | 4 ++-
.../btrfs_raid_single_disk.json | 3 ++
.../btrfs_raid_single_disk.toml | 15 +++++++++
.../zfs_raid_single_disk.json | 3 ++
.../zfs_raid_single_disk.toml | 15 +++++++++
proxmox-installer-common/src/options.rs | 32 +++++++++++++++++++
8 files changed, 90 insertions(+), 3 deletions(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index 6ba6617..584a622 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -20,7 +20,7 @@ use proxmox_auto_installer::{
sysinfo::SysInfo,
utils::{
AutoInstSettings, FetchAnswerFrom, HttpOptions, default_partition_label,
- get_matched_udev_indexes, get_nic_list, get_single_udev_index,
+ get_matched_udev_indexes, get_nic_list, get_single_udev_index, verify_disks_settings,
verify_email_and_root_password_settings, verify_first_boot_settings,
verify_locale_settings,
},
@@ -817,6 +817,7 @@ fn parse_answer(path: impl AsRef<Path> + fmt::Debug) -> Result<Answer> {
match toml::from_str(&contents) {
Ok(answer) => {
verify_locale_settings(&answer, &serde_json::from_str(LOCALE_INFO)?)?;
+ verify_disks_settings(&answer)?;
verify_first_boot_settings(&answer)?;
verify_email_and_root_password_settings(&answer)?;
println!("The answer file was parsed successfully, no errors found!");
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index e049748..af119e2 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -5,7 +5,8 @@ use std::{collections::BTreeMap, process::Command};
use crate::{
answer::{
- self, Answer, FirstBootHookSourceMode, FqdnConfig, FqdnExtendedConfig, FqdnSourceMode,
+ self, Answer, DiskSelection, FirstBootHookSourceMode, FqdnConfig, FqdnExtendedConfig,
+ FqdnSourceMode,
},
udevinfo::UdevInfo,
};
@@ -385,6 +386,20 @@ pub fn verify_email_and_root_password_settings(answer: &Answer) -> Result<()> {
}
}
+pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
+ if let DiskSelection::Selection(selection) = &answer.disks.disk_selection {
+ let min_disks = answer.disks.fs_type.get_min_disks();
+ if selection.len() < min_disks {
+ bail!(
+ "{} requires at least {} disks",
+ answer.disks.fs_type,
+ min_disks
+ );
+ }
+ }
+ Ok(())
+}
+
pub fn verify_first_boot_settings(answer: &Answer) -> Result<()> {
info!("Verifying first boot settings");
@@ -415,6 +430,7 @@ pub fn parse_answer(
let network_settings = get_network_settings(answer, udev_info, runtime_info, setup_info)?;
verify_locale_settings(answer, locales)?;
+ verify_disks_settings(answer)?;
verify_email_and_root_password_settings(answer)?;
verify_first_boot_settings(answer)?;
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 34bc969..92dba63 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -142,11 +142,13 @@ mod tests {
run_named_fail_parse_test,
// Keep below entries alphabetically sorted
both_password_and_hashed_set,
+ btrfs_raid_single_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
no_fqdn_from_dhcp,
no_root_password_set,
- short_password
+ short_password,
+ zfs_raid_single_disk
);
}
}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
new file mode 100644
index 0000000..37f35fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
@@ -0,0 +1,3 @@
+{
+ "error": "BTRFS (RAID1) requires at least 2 disks"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
new file mode 100644
index 0000000..e96fb16
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "btrfs"
+btrfs.raid = "raid1"
+disk-list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
new file mode 100644
index 0000000..9a8cc90
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
@@ -0,0 +1,3 @@
+{
+ "error": "ZFS (RAID10) requires at least 4 disks"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
new file mode 100644
index 0000000..94ae748
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "zfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "zfs"
+zfs.raid = "raid10"
+disk-list = ["sda"]
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9cc4ee0..9271b8b 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -20,6 +20,16 @@ pub enum BtrfsRaidLevel {
Raid10,
}
+impl BtrfsRaidLevel {
+ pub fn get_min_disks(&self) -> usize {
+ match self {
+ BtrfsRaidLevel::Raid0 => 1,
+ BtrfsRaidLevel::Raid1 => 2,
+ BtrfsRaidLevel::Raid10 => 4,
+ }
+ }
+}
+
serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
#[derive(Copy, Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
@@ -48,6 +58,19 @@ pub enum ZfsRaidLevel {
RaidZ3,
}
+impl ZfsRaidLevel {
+ pub fn get_min_disks(&self) -> usize {
+ match self {
+ ZfsRaidLevel::Raid0 => 1,
+ ZfsRaidLevel::Raid1 => 2,
+ ZfsRaidLevel::Raid10 => 4,
+ ZfsRaidLevel::RaidZ => 3,
+ ZfsRaidLevel::RaidZ2 => 4,
+ ZfsRaidLevel::RaidZ3 => 5,
+ }
+ }
+}
+
serde_plain::derive_display_from_serialize!(ZfsRaidLevel);
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -67,6 +90,15 @@ impl FsType {
pub fn is_lvm(&self) -> bool {
matches!(self, FsType::Ext4 | FsType::Xfs)
}
+
+ pub fn get_min_disks(&self) -> usize {
+ match self {
+ FsType::Ext4 => 1,
+ FsType::Xfs => 1,
+ FsType::Zfs(level) => level.get_min_disks(),
+ FsType::Btrfs(level) => level.get_min_disks(),
+ }
+ }
}
impl fmt::Display for FsType {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 1/7] auto: add early answer file sanity check for RAID configurations Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-07-07 11:47 ` Christoph Heiss
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize Michael Köppl
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
Instead of having parts of the RAID setup checks scattered in multiple
places, move the core of the checks to implementations of the
ZfsRaidLevel and BtrfsRaidLevel enums.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
No functional change intended.
proxmox-installer-common/src/disk_checks.rs | 156 ++++----------------
proxmox-installer-common/src/options.rs | 59 ++++++++
proxmox-tui-installer/src/views/bootdisk.rs | 13 +-
3 files changed, 96 insertions(+), 132 deletions(-)
diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index ecc43bd..d535837 100644
--- a/proxmox-installer-common/src/disk_checks.rs
+++ b/proxmox-installer-common/src/disk_checks.rs
@@ -1,6 +1,6 @@
use std::collections::HashSet;
-use crate::options::{BtrfsRaidLevel, Disk, ZfsRaidLevel};
+use crate::options::Disk;
use crate::setup::BootType;
/// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,94 +49,10 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
Ok(())
}
-/// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
-/// number of disks.
-///
-/// # Arguments
-///
-/// * `level` - The targeted ZFS RAID level by the user.
-/// * `disks` - List of disks designated as RAID targets.
-pub fn check_zfs_raid_config(level: ZfsRaidLevel, disks: &[Disk]) -> Result<(), String> {
- // See also Proxmox/Install.pm:get_zfs_raid_setup()
-
- let check_mirror_size = |disk1: &Disk, disk2: &Disk| {
- if (disk1.size - disk2.size).abs() > disk1.size / 10. {
- Err(format!(
- "Mirrored disks must have same size:\n\n * {disk1}\n * {disk2}"
- ))
- } else {
- Ok(())
- }
- };
-
- match level {
- ZfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
- ZfsRaidLevel::Raid1 => {
- check_raid_min_disks(disks, 2)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- ZfsRaidLevel::Raid10 => {
- check_raid_min_disks(disks, 4)?;
-
- if disks.len() % 2 != 0 {
- return Err(format!(
- "Needs an even number of disks, currently selected: {}",
- disks.len(),
- ));
- }
-
- // Pairs need to have the same size
- for i in (0..disks.len()).step_by(2) {
- check_mirror_size(&disks[i], &disks[i + 1])?;
- }
- }
- // For RAID-Z: minimum disks number is level + 2
- ZfsRaidLevel::RaidZ => {
- check_raid_min_disks(disks, 3)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- ZfsRaidLevel::RaidZ2 => {
- check_raid_min_disks(disks, 4)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- ZfsRaidLevel::RaidZ3 => {
- check_raid_min_disks(disks, 5)?;
- for disk in disks {
- check_mirror_size(&disks[0], disk)?;
- }
- }
- }
-
- Ok(())
-}
-
-/// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
-/// number of disks.
-///
-/// # Arguments
-///
-/// * `level` - The targeted Btrfs RAID level by the user.
-/// * `disks` - List of disks designated as RAID targets.
-pub fn check_btrfs_raid_config(level: BtrfsRaidLevel, disks: &[Disk]) -> Result<(), String> {
- // See also Proxmox/Install.pm:get_btrfs_raid_setup()
-
- match level {
- BtrfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
- BtrfsRaidLevel::Raid1 => check_raid_min_disks(disks, 2)?,
- BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 4)?,
- }
-
- Ok(())
-}
-
#[cfg(test)]
mod tests {
+ use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
+
use super::*;
fn dummy_disk(index: usize) -> Disk {
@@ -194,50 +110,38 @@ mod tests {
fn btrfs_raid() {
let disks = dummy_disks(10);
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &[]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks[..1]).is_ok());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks).is_ok());
-
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &[]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..1]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..2]).is_ok());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks).is_ok());
-
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &[]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..3]).is_err());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..4]).is_ok());
- assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks).is_ok());
+ let btrfs_raid_variants = [
+ BtrfsRaidLevel::Raid0,
+ BtrfsRaidLevel::Raid1,
+ BtrfsRaidLevel::Raid10,
+ ];
+
+ for v in btrfs_raid_variants {
+ assert!(v.check_disks(&[]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
+ assert!(v.check_disks(&disks).is_ok());
+ }
}
#[test]
fn zfs_raid() {
let disks = dummy_disks(10);
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks[..1]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks[..2]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &dummy_disks(4)).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..2]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..3]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..3]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..4]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks).is_ok());
-
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &[]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..4]).is_err());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..5]).is_ok());
- assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks).is_ok());
+ let zfs_raid_variants = [
+ ZfsRaidLevel::Raid0,
+ ZfsRaidLevel::Raid1,
+ ZfsRaidLevel::Raid10,
+ ZfsRaidLevel::RaidZ,
+ ZfsRaidLevel::RaidZ2,
+ ZfsRaidLevel::RaidZ3,
+ ];
+
+ for v in zfs_raid_variants {
+ assert!(v.check_disks(&[]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
+ assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
+ assert!(v.check_disks(&disks).is_ok());
+ }
}
}
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9271b8b..0552954 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -6,6 +6,7 @@ use std::str::FromStr;
use std::sync::OnceLock;
use std::{cmp, fmt};
+use crate::disk_checks::check_raid_min_disks;
use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
use crate::utils::{CidrAddress, Fqdn};
@@ -28,6 +29,17 @@ impl BtrfsRaidLevel {
BtrfsRaidLevel::Raid10 => 4,
}
}
+
+ /// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
+ /// number of disks.
+ ///
+ /// # Arguments
+ ///
+ /// * `disks` - List of disks designated as RAID targets.
+ pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
+ check_raid_min_disks(disks, self.get_min_disks())?;
+ Ok(())
+ }
}
serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
@@ -69,6 +81,53 @@ impl ZfsRaidLevel {
ZfsRaidLevel::RaidZ3 => 5,
}
}
+
+ fn check_mirror_size(&self, disk1: &Disk, disk2: &Disk) -> Result<(), String> {
+ if (disk1.size - disk2.size).abs() > disk1.size / 10. {
+ Err(format!(
+ "Mirrored disks must have same size:\n\n * {disk1}\n * {disk2}"
+ ))
+ } else {
+ Ok(())
+ }
+ }
+
+ /// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
+ /// number of disks.
+ ///
+ /// # Arguments
+ ///
+ /// * `disks` - List of disks designated as RAID targets.
+ pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
+ check_raid_min_disks(disks, self.get_min_disks())?;
+
+ match self {
+ ZfsRaidLevel::Raid0 => {}
+ ZfsRaidLevel::Raid10 => {
+ if disks.len() % 2 != 0 {
+ return Err(format!(
+ "Needs an even number of disks, currently selected: {}",
+ disks.len(),
+ ));
+ }
+
+ // Pairs need to have the same size
+ for i in (0..disks.len()).step_by(2) {
+ self.check_mirror_size(&disks[i], &disks[i + 1])?;
+ }
+ }
+ ZfsRaidLevel::Raid1
+ | ZfsRaidLevel::RaidZ
+ | ZfsRaidLevel::RaidZ2
+ | ZfsRaidLevel::RaidZ3 => {
+ for disk in disks {
+ self.check_mirror_size(&disks[0], disk)?;
+ }
+ }
+ }
+
+ Ok(())
+ }
}
serde_plain::derive_display_from_serialize!(ZfsRaidLevel);
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 313a3c9..e87b040 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -17,10 +17,7 @@ use crate::InstallerState;
use crate::options::FS_TYPES;
use proxmox_installer_common::{
- disk_checks::{
- check_btrfs_raid_config, check_disks_4kn_legacy_boot, check_for_duplicate_disks,
- check_zfs_raid_config,
- },
+ disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
options::{
AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
@@ -275,7 +272,9 @@ impl AdvancedBootdiskOptionsView {
.ok_or("Failed to retrieve advanced bootdisk options")?;
if let FsType::Zfs(level) = fstype {
- check_zfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?;
+ level
+ .check_disks(&disks)
+ .map_err(|err| format!("{fstype}: {err}"))?;
}
Ok(BootdiskOptions {
@@ -289,7 +288,9 @@ impl AdvancedBootdiskOptionsView {
.ok_or("Failed to retrieve advanced bootdisk options")?;
if let FsType::Btrfs(level) = fstype {
- check_btrfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?;
+ level
+ .check_disks(&disks)
+ .map_err(|err| format!("{fstype}: {err}"))?;
}
Ok(BootdiskOptions {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations Michael Köppl
@ 2025-07-07 11:47 ` Christoph Heiss
2025-07-08 12:01 ` Michael Köppl
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Heiss @ 2025-07-07 11:47 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
[..]
> diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
> index ecc43bd..d535837 100644
> --- a/proxmox-installer-common/src/disk_checks.rs
> +++ b/proxmox-installer-common/src/disk_checks.rs
[..]
> #[cfg(test)]
> mod tests {
> + use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
> +
> use super::*;
>
> fn dummy_disk(index: usize) -> Disk {
> @@ -194,50 +110,38 @@ mod tests {
> fn btrfs_raid() {
> let disks = dummy_disks(10);
>
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &[]).is_err());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks[..1]).is_ok());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks).is_ok());
> -
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &[]).is_err());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..1]).is_err());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..2]).is_ok());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks).is_ok());
> -
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &[]).is_err());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..3]).is_err());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..4]).is_ok());
> - assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks).is_ok());
> + let btrfs_raid_variants = [
> + BtrfsRaidLevel::Raid0,
> + BtrfsRaidLevel::Raid1,
> + BtrfsRaidLevel::Raid10,
> + ];
> +
> + for v in btrfs_raid_variants {
> + assert!(v.check_disks(&[]).is_err());
> + assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
> + assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
> + assert!(v.check_disks(&disks).is_ok());
> + }
> }
>
> #[test]
> fn zfs_raid() {
> let disks = dummy_disks(10);
>
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &[]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks[..1]).is_ok());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks).is_ok());
> -
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &[]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks[..2]).is_ok());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks).is_ok());
> -
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &[]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &dummy_disks(4)).is_ok());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &disks).is_ok());
> -
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &[]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..2]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..3]).is_ok());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks).is_ok());
> -
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &[]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..3]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..4]).is_ok());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks).is_ok());
> -
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &[]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..4]).is_err());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..5]).is_ok());
> - assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks).is_ok());
> + let zfs_raid_variants = [
> + ZfsRaidLevel::Raid0,
> + ZfsRaidLevel::Raid1,
> + ZfsRaidLevel::Raid10,
> + ZfsRaidLevel::RaidZ,
> + ZfsRaidLevel::RaidZ2,
> + ZfsRaidLevel::RaidZ3,
> + ];
> +
> + for v in zfs_raid_variants {
> + assert!(v.check_disks(&[]).is_err());
> + assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
> + assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
> + assert!(v.check_disks(&disks).is_ok());
> + }
> }
These unit tests should be moved to `proxmox_installer_common::options`,
if the implementation of these methods also resides there.
> }
> diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
> index 9271b8b..0552954 100644
> --- a/proxmox-installer-common/src/options.rs
> +++ b/proxmox-installer-common/src/options.rs
> @@ -6,6 +6,7 @@ use std::str::FromStr;
> use std::sync::OnceLock;
> use std::{cmp, fmt};
>
> +use crate::disk_checks::check_raid_min_disks;
> use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
> use crate::utils::{CidrAddress, Fqdn};
>
> @@ -28,6 +29,17 @@ impl BtrfsRaidLevel {
> BtrfsRaidLevel::Raid10 => 4,
> }
> }
> +
> + /// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
> + /// number of disks.
> + ///
> + /// # Arguments
> + ///
> + /// * `disks` - List of disks designated as RAID targets.
> + pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
Maybe rename this to something more expressive, e.g.
check_raid_disks_setup()?
check_disks() by itself is a rather "opaque" method name and would (at
least to me, if I didn't know the implementation) suggests that the
actual disks are checked, not just the RAID configuration and sizes.
> + check_raid_min_disks(disks, self.get_min_disks())?;
> + Ok(())
> + }
> }
>
> serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
> @@ -69,6 +81,53 @@ impl ZfsRaidLevel {
> ZfsRaidLevel::RaidZ3 => 5,
> }
> }
> +
> + fn check_mirror_size(&self, disk1: &Disk, disk2: &Disk) -> Result<(), String> {
> + if (disk1.size - disk2.size).abs() > disk1.size / 10. {
> + Err(format!(
> + "Mirrored disks must have same size:\n\n * {disk1}\n * {disk2}"
> + ))
> + } else {
> + Ok(())
> + }
> + }
> +
> + /// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
> + /// number of disks.
> + ///
> + /// # Arguments
> + ///
> + /// * `disks` - List of disks designated as RAID targets.
> + pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
^ Same here as above.
> + check_raid_min_disks(disks, self.get_min_disks())?;
> +
> + match self {
> + ZfsRaidLevel::Raid0 => {}
> + ZfsRaidLevel::Raid10 => {
> + if disks.len() % 2 != 0 {
> + return Err(format!(
> + "Needs an even number of disks, currently selected: {}",
> + disks.len(),
> + ));
> + }
> +
> + // Pairs need to have the same size
> + for i in (0..disks.len()).step_by(2) {
> + self.check_mirror_size(&disks[i], &disks[i + 1])?;
> + }
> + }
> + ZfsRaidLevel::Raid1
> + | ZfsRaidLevel::RaidZ
> + | ZfsRaidLevel::RaidZ2
> + | ZfsRaidLevel::RaidZ3 => {
> + for disk in disks {
> + self.check_mirror_size(&disks[0], disk)?;
> + }
> + }
> + }
> +
> + Ok(())
> + }
> }
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations
2025-07-07 11:47 ` Christoph Heiss
@ 2025-07-08 12:01 ` Michael Köppl
2025-07-08 13:46 ` Christoph Heiss
0 siblings, 1 reply; 18+ messages in thread
From: Michael Köppl @ 2025-07-08 12:01 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 7/7/25 13:47, Christoph Heiss wrote:
>> +
>> + for v in zfs_raid_variants {
>> + assert!(v.check_disks(&[]).is_err());
>> + assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
>> + assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
>> + assert!(v.check_disks(&disks).is_ok());
>> + }
>> }
>
> These unit tests should be moved to `proxmox_installer_common::options`,
> if the implementation of these methods also resides there.
First of all, thanks for having a look at this! True, I forgot to move
the tests as well. Will move them for v4. Right now, both sets of unit
tests (for proxmox_installer_common::options and
proxmox_installer_common::disk_checks) use the dummy_disks and
dummy_disk helpers. I think simply copying the implementation of these
over to the options.rs tests is fine since the implementation is not
exactly complicated and it seems a bit overkill to introduce some sort
of test utils for this. What do you think?
>
>> }
>> diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
>> index 9271b8b..0552954 100644
>> --- a/proxmox-installer-common/src/options.rs
>> +++ b/proxmox-installer-common/src/options.rs
>> @@ -6,6 +6,7 @@ use std::str::FromStr;
>> use std::sync::OnceLock;
>> use std::{cmp, fmt};
>>
>> +use crate::disk_checks::check_raid_min_disks;
>> use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
>> use crate::utils::{CidrAddress, Fqdn};
>>
>> @@ -28,6 +29,17 @@ impl BtrfsRaidLevel {
>> BtrfsRaidLevel::Raid10 => 4,
>> }
>> }
>> +
>> + /// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
>> + /// number of disks.
>> + ///
>> + /// # Arguments
>> + ///
>> + /// * `disks` - List of disks designated as RAID targets.
>> + pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
>
> Maybe rename this to something more expressive, e.g.
> check_raid_disks_setup()?
>
> check_disks() by itself is a rather "opaque" method name and would (at
> least to me, if I didn't know the implementation) suggests that the
> actual disks are checked, not just the RAID configuration and sizes.
I agree that check_disks() suggests that the function does
more/something different than it actually does. Thanks for your
suggestion, I'll adapt it to check_raid_disks_setup() in v4.
>
>> + check_raid_min_disks(disks, self.get_min_disks())?;
>> + Ok(())
>> + }
>> }
>>
>> serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
>> @@ -69,6 +81,53 @@ impl ZfsRaidLevel {
>> ZfsRaidLevel::RaidZ3 => 5,
>> }
>> }
>> +
>> + fn check_mirror_size(&self, disk1: &Disk, disk2: &Disk) -> Result<(), String> {
>> + if (disk1.size - disk2.size).abs() > disk1.size / 10. {
>> + Err(format!(
>> + "Mirrored disks must have same size:\n\n * {disk1}\n * {disk2}"
>> + ))
>> + } else {
>> + Ok(())
>> + }
>> + }
>> +
>> + /// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
>> + /// number of disks.
>> + ///
>> + /// # Arguments
>> + ///
>> + /// * `disks` - List of disks designated as RAID targets.
>> + pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
>
> ^ Same here as above.
>
>> + check_raid_min_disks(disks, self.get_min_disks())?;
>> +
>> + match self {
>> + ZfsRaidLevel::Raid0 => {}
>> + ZfsRaidLevel::Raid10 => {
>> + if disks.len() % 2 != 0 {
>> + return Err(format!(
>> + "Needs an even number of disks, currently selected: {}",
>> + disks.len(),
>> + ));
>> + }
>> +
>> + // Pairs need to have the same size
>> + for i in (0..disks.len()).step_by(2) {
>> + self.check_mirror_size(&disks[i], &disks[i + 1])?;
>> + }
>> + }
>> + ZfsRaidLevel::Raid1
>> + | ZfsRaidLevel::RaidZ
>> + | ZfsRaidLevel::RaidZ2
>> + | ZfsRaidLevel::RaidZ3 => {
>> + for disk in disks {
>> + self.check_mirror_size(&disks[0], disk)?;
>> + }
>> + }
>> + }
>> +
>> + Ok(())
>> + }
>> }
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations
2025-07-08 12:01 ` Michael Köppl
@ 2025-07-08 13:46 ` Christoph Heiss
2025-07-08 14:36 ` Michael Köppl
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Heiss @ 2025-07-08 13:46 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
On Tue Jul 8, 2025 at 2:01 PM CEST, Michael Köppl wrote:
> On 7/7/25 13:47, Christoph Heiss wrote:
>>> +
>>> + for v in zfs_raid_variants {
>>> + assert!(v.check_disks(&[]).is_err());
>>> + assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
>>> + assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
>>> + assert!(v.check_disks(&disks).is_ok());
>>> + }
>>> }
>>
>> These unit tests should be moved to `proxmox_installer_common::options`,
>> if the implementation of these methods also resides there.
>
> First of all, thanks for having a look at this! True, I forgot to move
> the tests as well. Will move them for v4. Right now, both sets of unit
> tests (for proxmox_installer_common::options and
> proxmox_installer_common::disk_checks) use the dummy_disks and
> dummy_disk helpers. I think simply copying the implementation of these
> over to the options.rs tests is fine since the implementation is not
> exactly complicated and it seems a bit overkill to introduce some sort
> of test utils for this. What do you think?
As for dummy_disk(), you could potentially make it a (static) method on
the `Disk` type itself, gating it on `#[cfg(test)]`.
That would avoid some extra test utilities somewhere, while still having
the benefit of being able to deduplicate it (even if trivial).
We already use a similar pattern already in the setup code, see e.g.
proxmox_installer_common::setup::ProductConfig::mocked()
dummy_disks() being a one-liner can of course be simply copied over.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations
2025-07-08 13:46 ` Christoph Heiss
@ 2025-07-08 14:36 ` Michael Köppl
0 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-07-08 14:36 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 7/8/25 15:46, Christoph Heiss wrote:
> As for dummy_disk(), you could potentially make it a (static) method on
> the `Disk` type itself, gating it on `#[cfg(test)]`.
>
> That would avoid some extra test utilities somewhere, while still having
> the benefit of being able to deduplicate it (even if trivial).
> We already use a similar pattern already in the setup code, see e.g.
> proxmox_installer_common::setup::ProductConfig::mocked()
>
> dummy_disks() being a one-liner can of course be simply copied over.
Thanks for pointing me in the right direction. I like this approach,
will adapt for v4!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 1/7] auto: add early answer file sanity check for RAID configurations Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-07-07 12:07 ` Christoph Heiss
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 4/7] auto: add check for duplicate disks in answer file Michael Köppl
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
Check that the configured swapsize is not greater than hdsize / 8 as
stated in the admin guide [0]. Define the behavior for the auto-installer as
well as the TUI and GUI installers.
[0] https://pve.proxmox.com/pve-docs/pve-admin-guide.html#advanced_lvm_options
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Proxmox/Install.pm | 8 +++++
proxinstall | 4 ++-
proxmox-auto-installer/src/utils.rs | 10 ++++++
proxmox-auto-installer/tests/parse-answer.rs | 1 +
.../lvm_swapsize_greater_than_hdsize.json | 3 ++
.../lvm_swapsize_greater_than_hdsize.toml | 16 ++++++++++
proxmox-installer-common/src/disk_checks.rs | 32 ++++++++++++++++++-
proxmox-tui-installer/src/views/bootdisk.rs | 6 +++-
8 files changed, 77 insertions(+), 3 deletions(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 93cfc76..44882cf 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -599,6 +599,14 @@ sub compute_swapsize {
return $swapsize_kb;
}
+sub swapsize_check {
+ my ($hdsize) = @_;
+ my $swapsize = Proxmox::Install::Config::get_swapsize();
+ my $threshold = $hdsize / 8;
+ die "swap size ${swapsize} GiB cannot be greater than ${threshold} GiB (hard disk size / 8)\n"
+ if $swapsize > $threshold;
+}
+
my sub chroot_chown {
my ($root, $path, %param) = @_;
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");
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);
+ }
+ }
+ }
+
Ok(())
}
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 92dba63..ca8c09f 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -145,6 +145,7 @@ mod tests {
btrfs_raid_single_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
+ lvm_swapsize_greater_than_hdsize,
no_fqdn_from_dhcp,
no_root_password_set,
short_password,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
new file mode 100644
index 0000000..aa4f7fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
@@ -0,0 +1,3 @@
+{
+ "error": "Swap size 4.01 GiB cannot be greater than 4 GiB (hard disk size / 8)"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
new file mode 100644
index 0000000..ffe16dc
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
@@ -0,0 +1,16 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "ext4"
+lvm.swapsize = 4.01
+lvm.hdsize = 32
+disk-list = ["sda"]
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
@@ -1,6 +1,6 @@
use std::collections::HashSet;
-use crate::options::Disk;
+use crate::options::{Disk, LvmBootdiskOptions};
use crate::setup::BootType;
/// Checks a list of disks for duplicate entries, using their index as key.
@@ -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(())
+}
+
+/// Checks whether a user-supplied LVM setup is valid or not, such as the swapsize not
+/// exceeding a certain threshold.
+///
+/// # Arguments
+///
+/// * `bootdisk_opts` - The LVM options set by the user.
+pub fn check_lvm_bootdisk_opts(bootdisk_opts: &LvmBootdiskOptions) -> Result<(), String> {
+ if let Some(swap_size) = bootdisk_opts.swap_size {
+ check_swapsize(swap_size, bootdisk_opts.total_size)?;
+ }
+
+ Ok(())
+}
+
#[cfg(test)]
mod tests {
use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
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}"))?;
+
Ok(BootdiskOptions {
disks: vec![disk],
fstype,
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize Michael Köppl
@ 2025-07-07 12:07 ` Christoph Heiss
2025-07-08 17:45 ` Michael Köppl
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Heiss @ 2025-07-07 12:07 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize
2025-07-07 12:07 ` Christoph Heiss
@ 2025-07-08 17:45 ` Michael Köppl
0 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-07-08 17:45 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 7/7/25 14:07, Christoph Heiss wrote:
> 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.
Yes, absolutely. I'll update the error messages for v4. Makes sense to
do this now as it also makes for a more polished user experience. I
think in general there are many errors messages across the TUI and GUI
installers that could maybe be updated in the future or at least
unified. There are also cases where in one of the installers you get
details about an error and in the other you don't.
>
>> 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)
Ack. Thanks for the hint
>
>> + }
>> + }
>> +
>> 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.
Ack, makes sense to already use anyhow here then if it'll be used across
the entire installer repo in the long-term. Thanks, will update for v4.
>
> [..]
>> 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.
Ack
>
>> +
>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH pve-installer v3 4/7] auto: add check for duplicate disks in answer file
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
` (2 preceding siblings ...)
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 3/7] close #5887: add sanity check for LVM swapsize Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 5/7] common: add more descriptive errors for invalid network configs Michael Köppl
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-auto-installer/src/utils.rs | 12 +++++++++++-
proxmox-auto-installer/tests/parse-answer.rs | 1 +
.../parse_answer_fail/duplicate_disk.json | 3 +++
.../parse_answer_fail/duplicate_disk.toml | 15 +++++++++++++++
4 files changed, 30 insertions(+), 1 deletion(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index f29e1d4..6f61ebd 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -1,7 +1,10 @@
use anyhow::{Context, Result, bail};
use glob::Pattern;
use log::info;
-use std::{collections::BTreeMap, process::Command};
+use std::{
+ collections::{BTreeMap, HashSet},
+ process::Command,
+};
use crate::{
answer::{
@@ -397,6 +400,13 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
min_disks
);
}
+
+ let mut disk_set = HashSet::new();
+ for disk in selection {
+ if !disk_set.insert(disk) {
+ bail!("List of disks contains duplicate disk {disk}");
+ }
+ }
}
if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index ca8c09f..8b7eac4 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -143,6 +143,7 @@ mod tests {
// Keep below entries alphabetically sorted
both_password_and_hashed_set,
btrfs_raid_single_disk,
+ duplicate_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
lvm_swapsize_greater_than_hdsize,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
new file mode 100644
index 0000000..d01fabe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
@@ -0,0 +1,3 @@
+{
+ "error": "List of disks contains duplicate disk sda"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
new file mode 100644
index 0000000..bff1631
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "btrfs"
+btrfs.raid = "raid1"
+disk-list = ["sda", "sdb", "sda"]
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* [pve-devel] [PATCH pve-installer v3 5/7] common: add more descriptive errors for invalid network configs
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
` (3 preceding siblings ...)
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 4/7] auto: add check for duplicate disks in answer file Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-07-07 12:37 ` Christoph Heiss
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 6/7] tui: change get_value return type for easier error handling Michael Köppl
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-installer-common/src/utils.rs | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 8adcec0..1fe6a74 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -18,6 +18,20 @@ pub enum CidrAddressParseError {
InvalidMask(Option<ParseIntError>),
}
+impl fmt::Display for CidrAddressParseError {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ let msg = match &self {
+ CidrAddressParseError::NoDelimiter => {
+ String::from("No delimiter for separating address and mask was found")
+ }
+ CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
+ CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
+ };
+
+ write!(f, "Invalid CIDR: {msg}")
+ }
+}
+
/// An IP address (IPv4 or IPv6), including network mask.
///
/// See the [`IpAddr`] type for more information how IP addresses are handled.
@@ -109,8 +123,7 @@ impl<'de> Deserialize<'de> for CidrAddress {
D: serde::Deserializer<'de>,
{
let s: String = Deserialize::deserialize(deserializer)?;
- s.parse()
- .map_err(|_| serde::de::Error::custom("invalid CIDR"))
+ s.parse().map_err(serde::de::Error::custom)
}
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 5/7] common: add more descriptive errors for invalid network configs
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 5/7] common: add more descriptive errors for invalid network configs Michael Köppl
@ 2025-07-07 12:37 ` Christoph Heiss
2025-07-08 17:34 ` Michael Köppl
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Heiss @ 2025-07-07 12:37 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> proxmox-installer-common/src/utils.rs | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
> index 8adcec0..1fe6a74 100644
> --- a/proxmox-installer-common/src/utils.rs
> +++ b/proxmox-installer-common/src/utils.rs
> @@ -18,6 +18,20 @@ pub enum CidrAddressParseError {
> InvalidMask(Option<ParseIntError>),
> }
>
> +impl fmt::Display for CidrAddressParseError {
> + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> + let msg = match &self {
> + CidrAddressParseError::NoDelimiter => {
> + String::from("No delimiter for separating address and mask was found")
> + }
> + CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
> + CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
> + };
> +
> + write!(f, "Invalid CIDR: {msg}")
> + }
> +}
This implementation is refactored/rewritten in patch #7 to directly use
write!() everywhere - this should already be done here, instead of
implementing it here differently and later rewriting it.
> +
> /// An IP address (IPv4 or IPv6), including network mask.
> ///
> /// See the [`IpAddr`] type for more information how IP addresses are handled.
> @@ -109,8 +123,7 @@ impl<'de> Deserialize<'de> for CidrAddress {
> D: serde::Deserializer<'de>,
> {
> let s: String = Deserialize::deserialize(deserializer)?;
> - s.parse()
> - .map_err(|_| serde::de::Error::custom("invalid CIDR"))
> + s.parse().map_err(serde::de::Error::custom)
> }
> }
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 5/7] common: add more descriptive errors for invalid network configs
2025-07-07 12:37 ` Christoph Heiss
@ 2025-07-08 17:34 ` Michael Köppl
0 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-07-08 17:34 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 7/7/25 14:37, Christoph Heiss wrote:
> On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>> proxmox-installer-common/src/utils.rs | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
>> index 8adcec0..1fe6a74 100644
>> --- a/proxmox-installer-common/src/utils.rs
>> +++ b/proxmox-installer-common/src/utils.rs
>> @@ -18,6 +18,20 @@ pub enum CidrAddressParseError {
>> InvalidMask(Option<ParseIntError>),
>> }
>>
>> +impl fmt::Display for CidrAddressParseError {
>> + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> + let msg = match &self {
>> + CidrAddressParseError::NoDelimiter => {
>> + String::from("No delimiter for separating address and mask was found")
>> + }
>> + CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
>> + CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
>> + };
>> +
>> + write!(f, "Invalid CIDR: {msg}")
>> + }
>> +}
>
> This implementation is refactored/rewritten in patch #7 to directly use
> write!() everywhere - this should already be done here, instead of
> implementing it here differently and later rewriting it.
>
Ack, will update in v4.
>> +
>> /// An IP address (IPv4 or IPv6), including network mask.
>> ///
>> /// See the [`IpAddr`] type for more information how IP addresses are handled.
>> @@ -109,8 +123,7 @@ impl<'de> Deserialize<'de> for CidrAddress {
>> D: serde::Deserializer<'de>,
>> {
>> let s: String = Deserialize::deserialize(deserializer)?;
>> - s.parse()
>> - .map_err(|_| serde::de::Error::custom("invalid CIDR"))
>> + s.parse().map_err(serde::de::Error::custom)
>> }
>> }
>>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH pve-installer v3 6/7] tui: change get_value return type for easier error handling
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
` (4 preceding siblings ...)
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 5/7] common: add more descriptive errors for invalid network configs Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-07-07 12:56 ` Christoph Heiss
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 7/7] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-docs v3 1/1] installation: remove maxroot size requirement and mention default instead Michael Köppl
7 siblings, 1 reply; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
Adapt the return type of CidrAddressEditView's get_value implementation
for the FormViewGetValue trait to handle errors in case of invalid CIDR
similarly to other (parsing) errors done in the TUIs network dialog.
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-tui-installer/src/main.rs | 8 +++++---
proxmox-tui-installer/src/views/mod.rs | 8 ++++----
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 57a334f..15ee5d3 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -20,9 +20,8 @@ use proxmox_installer_common::{
ROOT_PASSWORD_MIN_LENGTH,
options::{BootdiskOptions, NetworkOptions, TimezoneOptions, email_validate},
setup::{LocaleInfo, ProxmoxProduct, RuntimeInfo, SetupInfo, installer_setup},
- utils::Fqdn,
+ utils::{CidrAddress, Fqdn},
};
-
mod setup;
mod system;
@@ -536,7 +535,10 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView {
let address = view
.get_value::<CidrAddressEditView, _>(2)
- .ok_or("failed to retrieve host address")?;
+ .ok_or("failed to retrieve host address".to_string())
+ .and_then(|(ip_addr, mask)| {
+ CidrAddress::new(ip_addr, mask).map_err(|err| err.to_string())
+ })?;
let gateway = view
.get_value::<EditView, _>(3)
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 4364489..3211c93 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -387,8 +387,8 @@ where
}
}
-impl FormViewGetValue<CidrAddress> for CidrAddressEditView {
- fn get_value(&self) -> Option<CidrAddress> {
+impl FormViewGetValue<(IpAddr, usize)> for CidrAddressEditView {
+ fn get_value(&self) -> Option<(IpAddr, usize)> {
self.get_values()
}
}
@@ -569,7 +569,7 @@ impl CidrAddressEditView {
.fixed_width(4)
}
- fn get_values(&self) -> Option<CidrAddress> {
+ fn get_values(&self) -> Option<(IpAddr, usize)> {
let addr = self
.view
.get_child(0)?
@@ -587,7 +587,7 @@ impl CidrAddressEditView {
.get_content()
.ok()?;
- CidrAddress::new(addr, mask).ok()
+ Some((addr, mask))
}
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH pve-installer v3 6/7] tui: change get_value return type for easier error handling
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 6/7] tui: change get_value return type for easier error handling Michael Köppl
@ 2025-07-07 12:56 ` Christoph Heiss
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Heiss @ 2025-07-07 12:56 UTC (permalink / raw)
To: Michael Köppl; +Cc: Proxmox VE development discussion
On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
[..]
> diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
> index 4364489..3211c93 100644
> --- a/proxmox-tui-installer/src/views/mod.rs
> +++ b/proxmox-tui-installer/src/views/mod.rs
> @@ -387,8 +387,8 @@ where
> }
> }
>
> -impl FormViewGetValue<CidrAddress> for CidrAddressEditView {
> - fn get_value(&self) -> Option<CidrAddress> {
> +impl FormViewGetValue<(IpAddr, usize)> for CidrAddressEditView {
> + fn get_value(&self) -> Option<(IpAddr, usize)> {
I see _why_ you changed it, although it's a bit backwards - since it
makes sense that `CidrAddressEditView` would produce a `CidrAddress`.
The proper solution here is to change `FormViewGetValue::get_value()` to
return a `Result<R>` instead of an `Option<R>`, such that errors can
easily be bubbled up.
IIRC I started a series locally to do exactly that and address these
cases, but it's a bigger refactoring.
But for now, IMHO this would be okay as a stop-gap solution, instead of
holding up this series on a somewhat unrelated refactoring.
Can you please at least add a big FIXME here, that this implementation
should use `CidrAddress` (again) instead of a (IP, mask) tuple?
> self.get_values()
> }
> }
> @@ -569,7 +569,7 @@ impl CidrAddressEditView {
> .fixed_width(4)
> }
>
> - fn get_values(&self) -> Option<CidrAddress> {
> + fn get_values(&self) -> Option<(IpAddr, usize)> {
> let addr = self
> .view
> .get_child(0)?
> @@ -587,7 +587,7 @@ impl CidrAddressEditView {
> .get_content()
> .ok()?;
>
> - CidrAddress::new(addr, mask).ok()
> + Some((addr, mask))
> }
> }
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH pve-installer v3 7/7] common: add checks for valid subnet mask and IPv4 address within subnet
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
` (5 preceding siblings ...)
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 6/7] tui: change get_value return type for easier error handling Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
2025-06-26 15:11 ` [pve-devel] [PATCH pve-docs v3 1/1] installation: remove maxroot size requirement and mention default instead Michael Köppl
7 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
Add checks for valid subnet mask (greater than /0 and at most /32 for
IPv4). In addition, check if the address entered by the user is valid
within the given subnet, i.e. not a network address or broadcast
address. /31 is considered an exception in accordance with RFC3021 [0],
considering any of the 2 available addresses to be valid host addresses.
[0] https://datatracker.ietf.org/doc/html/rfc3021
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
proxmox-auto-installer/tests/parse-answer.rs | 3 +
.../parse_answer/ipv4_and_subnet_31.json | 19 +++++
.../parse_answer/ipv4_and_subnet_31.toml | 18 ++++
.../ipv4_and_subnet_addr_is_network.json | 3 +
.../ipv4_and_subnet_addr_is_network.toml | 18 ++++
.../ipv4_and_subnet_mask_33.json | 3 +
.../ipv4_and_subnet_mask_33.toml | 18 ++++
proxmox-installer-common/src/utils.rs | 82 ++++++++++++++-----
8 files changed, 144 insertions(+), 20 deletions(-)
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 8b7eac4..88f30aa 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -127,6 +127,7 @@ mod tests {
fqdn_from_dhcp_no_dhcp_domain_with_default_domain,
full_fqdn_from_dhcp_with_default_domain,
hashed_root_password,
+ ipv4_and_subnet_31,
minimal,
nic_matching,
specific_nic,
@@ -146,6 +147,8 @@ mod tests {
duplicate_disk,
fqdn_from_dhcp_no_default_domain,
fqdn_hostname_only,
+ ipv4_and_subnet_addr_is_network,
+ ipv4_and_subnet_mask_33,
lvm_swapsize_greater_than_hdsize,
no_fqdn_from_dhcp,
no_root_password_set,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
new file mode 100644
index 0000000..2a475fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
@@ -0,0 +1,19 @@
+{
+ "autoreboot": 1,
+ "cidr": "10.10.10.10/31",
+ "country": "at",
+ "dns": "10.10.10.1",
+ "domain": "testinstall",
+ "filesys": "ext4",
+ "gateway": "10.10.10.1",
+ "hdsize": 223.57088470458984,
+ "existing_storage_auto_rename": 1,
+ "hostname": "pveauto",
+ "keymap": "de",
+ "mailto": "mail@no.invalid",
+ "mngmt_nic": "enp129s0f1np1",
+ "root_password": { "plain": "12345678" },
+ "target_hd": "/dev/sda",
+ "timezone": "Europe/Vienna",
+ "first_boot": { "enabled": 0 }
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
new file mode 100644
index 0000000..0ca6ca5
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/31"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
new file mode 100644
index 0000000..b28699d
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
@@ -0,0 +1,3 @@
+{
+ "parse-error": "error parsing answer.toml: Invalid CIDR: address is a network address"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
new file mode 100644
index 0000000..7a9141b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.32/27"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
new file mode 100644
index 0000000..6b2888b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
@@ -0,0 +1,3 @@
+{
+ "parse-error": "error parsing answer.toml: Invalid CIDR: mask cannot be greater than 32"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
new file mode 100644
index 0000000..16a560f
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/33"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 1fe6a74..929cc6b 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -1,7 +1,7 @@
use std::{
+ error::Error,
fmt,
net::{AddrParseError, IpAddr},
- num::ParseIntError,
str::FromStr,
};
@@ -15,20 +15,26 @@ pub enum CidrAddressParseError {
/// The IP address part could not be parsed.
InvalidAddr(AddrParseError),
/// The mask could not be parsed.
- InvalidMask(Option<ParseIntError>),
+ InvalidMask(Box<dyn Error>),
+ /// The IP address is a network address.
+ IsNetworkAddr,
+ /// The IP address is a broadcast address.
+ IsBroadcastAddr,
}
impl fmt::Display for CidrAddressParseError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- let msg = match &self {
+ write!(f, "Invalid CIDR: ")?;
+
+ match self {
CidrAddressParseError::NoDelimiter => {
- String::from("No delimiter for separating address and mask was found")
+ write!(f, "no delimiter for separating address and mask was found")
}
- CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
- CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
- };
-
- write!(f, "Invalid CIDR: {msg}")
+ CidrAddressParseError::InvalidAddr(err) => write!(f, "{err}"),
+ CidrAddressParseError::InvalidMask(err) => write!(f, "{err}"),
+ CidrAddressParseError::IsNetworkAddr => write!(f, "address is a network address"),
+ CidrAddressParseError::IsBroadcastAddr => write!(f, "address is a broadcast address"),
+ }
}
}
@@ -61,11 +67,10 @@ impl CidrAddress {
pub fn new<T: Into<IpAddr>>(addr: T, mask: usize) -> Result<Self, CidrAddressParseError> {
let addr = addr.into();
- if mask > mask_limit(&addr) {
- Err(CidrAddressParseError::InvalidMask(None))
- } else {
- Ok(Self { addr, mask })
- }
+ check_mask_limit(&addr, mask)?;
+ check_addr_valid_in_subnet(&addr, mask)?;
+
+ Ok(Self { addr, mask })
}
/// Returns only the IP address part of the address.
@@ -101,13 +106,12 @@ impl FromStr for CidrAddress {
let mask = mask
.parse()
- .map_err(|err| CidrAddressParseError::InvalidMask(Some(err)))?;
+ .map_err(|err| CidrAddressParseError::InvalidMask(Box::new(err)))?;
- if mask > mask_limit(&addr) {
- Err(CidrAddressParseError::InvalidMask(None))
- } else {
- Ok(Self { addr, mask })
- }
+ check_mask_limit(&addr, mask)?;
+ check_addr_valid_in_subnet(&addr, mask)?;
+
+ Ok(Self { addr, mask })
}
}
@@ -133,6 +137,44 @@ fn mask_limit(addr: &IpAddr) -> usize {
if addr.is_ipv4() { 32 } else { 128 }
}
+fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+ let limit = mask_limit(&addr);
+ return if mask > limit {
+ Err(CidrAddressParseError::InvalidMask(
+ format!("mask cannot be greater than {limit}").into(),
+ ))
+ } else {
+ Ok(())
+ };
+}
+
+fn check_addr_valid_in_subnet(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+ match &addr {
+ IpAddr::V4(addr_v4) => {
+ if mask >= 31 {
+ return Ok(());
+ }
+
+ let num_host_bits = 32 - mask;
+ let host_part_mask = (1u32 << num_host_bits) - 1;
+
+ let ip_bits = u32::from_be_bytes(addr_v4.octets());
+
+ let network_addr = ip_bits & !host_part_mask;
+ let broadcast_addr = network_addr | host_part_mask;
+
+ if ip_bits == network_addr {
+ Err(CidrAddressParseError::IsNetworkAddr)
+ } else if ip_bits == broadcast_addr {
+ Err(CidrAddressParseError::IsBroadcastAddr)
+ } else {
+ Ok(())
+ }
+ }
+ IpAddr::V6(_) => Ok(()),
+ }
+}
+
/// Possible errors that might occur when parsing FQDNs.
#[derive(Debug, Eq, PartialEq)]
pub enum FqdnParseError {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* [pve-devel] [PATCH pve-docs v3 1/1] installation: remove maxroot size requirement and mention default instead
2025-06-26 15:11 [pve-devel] [PATCH docs/installer v3 0/8] add early disk and network sanity checks Michael Köppl
` (6 preceding siblings ...)
2025-06-26 15:11 ` [pve-devel] [PATCH pve-installer v3 7/7] common: add checks for valid subnet mask and IPv4 address within subnet Michael Köppl
@ 2025-06-26 15:11 ` Michael Köppl
7 siblings, 0 replies; 18+ messages in thread
From: Michael Köppl @ 2025-06-26 15:11 UTC (permalink / raw)
To: pve-devel
The requirement of hdsize/4 was not checked anywhere and adding sanity
checks for maxroot<=hdsize/4 would stop users from installing PVE on
smaller disks (see [0]), whereas the installation actually tries its
best to successfully install even on disks below 12GB. So instead of
adding sanity checks, relax the documentation regarding this requirement
and instead refer to the defaults used during installation to provide
guidance for users.
[0] https://lore.proxmox.com/pve-devel/D9P1YXB42LGJ.ULII1HUIPAWQ@proxmox.com/
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
The default size is not exact. The size of the root volume also depends
on the swap size and the installer will then add 12GiB to (os_size -
swap_size)/4. Was not entirely sure how to best describe this here
without going into too much technical details for users and without
making this part of the docs too confusing. The idea behind the
description in this patch was to give users some orientation in case
they want to choose the root size themselves. Feedback very much
appreciated.
pve-installation.adoc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/pve-installation.adoc b/pve-installation.adoc
index 223bb716..30ad6b81 100644
--- a/pve-installation.adoc
+++ b/pve-installation.adoc
@@ -281,7 +281,9 @@ NOTE: If set to `0`, no `swap` volume will be created.
`maxroot`::
Defines the maximum size of the `root` volume, which stores the operation
-system. The maximum limit of the `root` volume size is `hdsize/4`.
+system. With more than 48GiB storage available, the default is `hdsize/4`
+with a maximum of 96 GiB. Below 48GiB, the `root` volume size is at least
+`hdsize/2`.
`maxvz`::
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread