From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 0DC191FF0B4 for ; Tue, 08 Sep 2026 12:17:45 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 704CA214FF; Tue, 08 Sep 2026 12:17:38 +0200 (CEST) From: Christoph Heiss To: pve-devel@lists.proxmox.com Subject: [PATCH installer 04/15] tree-wide: use `MacAddress` type instead of string for MAC addresses Date: Tue, 8 Sep 2026 12:16:18 +0200 Message-ID: <20260908101647.1057780-5-c.heiss@proxmox.com> X-Mailer: git-send-email 2.55.0 In-Reply-To: <20260908101647.1057780-1-c.heiss@proxmox.com> References: <20260908101647.1057780-1-c.heiss@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788862631029 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.404 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: DTUFVQE7MOB6F5GU3TVOIQBZJKKQTWKH X-Message-ID-Hash: DTUFVQE7MOB6F5GU3TVOIQBZJKKQTWKH X-MailFrom: c.heiss@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Makes all Rust code dealing with MAC addresses a lot more typesafe. The `MacAddress` type is also cheaper to construct than `String`, since the former is just 6 bytes and avoids heap allocations. Signed-off-by: Christoph Heiss --- proxmox-auto-installer/tests/parse-answer.rs | 2 +- ...ace_pinning_mixed_case_mac_addresses.json} | 0 ...ace_pinning_mixed_case_mac_addresses.toml} | 4 +- ...rface_pinning_overlong_interface_name.json | 2 +- proxmox-installer-common/src/options.rs | 66 +++++++++---------- proxmox-installer-common/src/setup.rs | 28 ++++++-- proxmox-post-hook/src/main.rs | 2 +- proxmox-tui-installer/src/views/network.rs | 26 +++----- 8 files changed, 68 insertions(+), 62 deletions(-) rename proxmox-auto-installer/tests/resources/parse_answer/{network_interface_pinning_uppercase_mac_address.json => network_interface_pinning_mixed_case_mac_addresses.json} (100%) rename proxmox-auto-installer/tests/resources/parse_answer/{network_interface_pinning_uppercase_mac_address.toml => network_interface_pinning_mixed_case_mac_addresses.toml} (85%) diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs index 8b0bae7..ae5b87d 100644 --- a/proxmox-auto-installer/tests/parse-answer.rs +++ b/proxmox-auto-installer/tests/parse-answer.rs @@ -128,7 +128,7 @@ mod tests { hashed_root_password, minimal, network_interface_pinning, - network_interface_pinning_uppercase_mac_address, + network_interface_pinning_mixed_case_mac_addresses, nic_matching, no_network, specific_nic, diff --git a/proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_uppercase_mac_address.json b/proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_mixed_case_mac_addresses.json similarity index 100% rename from proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_uppercase_mac_address.json rename to proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_mixed_case_mac_addresses.json diff --git a/proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_uppercase_mac_address.toml b/proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_mixed_case_mac_addresses.toml similarity index 85% rename from proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_uppercase_mac_address.toml rename to proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_mixed_case_mac_addresses.toml index 6681fe9..deb1fd3 100644 --- a/proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_uppercase_mac_address.toml +++ b/proxmox-auto-installer/tests/resources/parse_answer/network_interface_pinning_mixed_case_mac_addresses.toml @@ -14,8 +14,8 @@ enabled = true [network.interface-name-pinning.mapping] "24:8A:07:1E:05:BC" = "lan0" -"24:8A:07:1E:05:BD" = "lan1" -"B4:2E:99:AC:AD:B4" = "mgmt" +"24:8a:07:1e:05:bd" = "lan1" +"B4:2e:99:ac:AD:b4" = "mgmt" [disk-setup] filesystem = "ext4" diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/network_interface_pinning_overlong_interface_name.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/network_interface_pinning_overlong_interface_name.json index f3c9169..952b34a 100644 --- a/proxmox-auto-installer/tests/resources/parse_answer_fail/network_interface_pinning_overlong_interface_name.json +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/network_interface_pinning_overlong_interface_name.json @@ -1,3 +1,3 @@ { - "error": "interface name 'waytoolonginterfacename' for 'ab:cd:ef:12:34:56' cannot be longer than 15 characters" + "error": "interface name 'waytoolonginterfacename' for 'AB:CD:EF:12:34:56' cannot be longer than 15 characters" } diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs index c7f0baf..fbbfe77 100644 --- a/proxmox-installer-common/src/options.rs +++ b/proxmox-installer-common/src/options.rs @@ -19,7 +19,7 @@ use proxmox_installer_types::{ ZfsChecksumOption, ZfsCompressOption, ZfsRaidLevel, }, }; -use proxmox_network_types::{fqdn::Fqdn, ip_address::Cidr}; +use proxmox_network_types::{MacAddress, fqdn::Fqdn, ip_address::Cidr}; pub trait RaidLevel { /// Returns the minimum number of disks needed for this RAID level. @@ -315,7 +315,7 @@ impl TimezoneOptions { pub struct NetworkInterfacePinningOptions { /// Maps MAC address to custom name #[serde(default)] - pub mapping: HashMap, + pub mapping: HashMap, } impl NetworkInterfacePinningOptions { @@ -341,7 +341,7 @@ impl NetworkInterfacePinningOptions { .unwrap() }); - let mut reverse_mapping = HashMap::::new(); + let mut reverse_mapping = HashMap::::new(); for (mac, name) in self.mapping.iter() { if name.len() < MIN_IFNAME_LEN { bail!( @@ -368,7 +368,7 @@ impl NetworkInterfacePinningOptions { bail!("duplicate interface name mapping '{name}' for: {mac}, {duplicate_mac}"); } - reverse_mapping.insert(name.clone(), mac.clone()); + reverse_mapping.insert(name.clone(), *mac); } Ok(()) @@ -379,13 +379,7 @@ impl From<&NetworkInterfacePinningOptionsAnswer> for NetworkInterfacePinningOpti fn from(answer: &NetworkInterfacePinningOptionsAnswer) -> Self { if answer.enabled { Self { - // convert all MAC addresses to lowercase before further usage, - // to enable easy comparison - mapping: answer - .mapping - .iter() - .map(|(k, v)| (k.to_lowercase(), v.clone())) - .collect(), + mapping: answer.mapping.clone(), } } else { Self::default() @@ -480,7 +474,7 @@ impl NetworkOptions { // required by the low-level installer for iface in network.interfaces.values() { if let Some(pinned) = iface.to_pinned(opts) { - opts.mapping.entry(iface.mac.clone()).or_insert(pinned.name); + opts.mapping.entry(iface.mac).or_insert(pinned.name); } } } @@ -609,7 +603,7 @@ mod tests { pinned_id: Some("0".to_owned()), state: InterfaceState::Up, driver: "dummy".to_owned(), - mac: "01:23:45:67:89:ab".to_owned(), + mac: "01:23:45:67:89:ab".parse().unwrap(), addresses: vec![Cidr::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap()], }, ); @@ -699,7 +693,7 @@ mod tests { pinned_id: Some("0".to_owned()), state: InterfaceState::Up, driver: "dummy".to_owned(), - mac: "01:23:45:67:89:ab".to_owned(), + mac: "01:23:45:67:89:ab".parse().unwrap(), addresses: vec![ Cidr::new(Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 2), 64).unwrap(), ], @@ -798,7 +792,7 @@ mod tests { pinned_id: Some("0".to_owned()), state: InterfaceState::Up, driver: "dummy".to_owned(), - mac: "01:23:45:67:89:ab".to_owned(), + mac: "01:23:45:67:89:ab".parse().unwrap(), addresses: vec![], }, ); @@ -836,13 +830,13 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), String::new()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), String::new()); let res = options.verify(); assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name for 'ab:cd:ef:12:34:56' must be at least 2 characters long" + "interface name for 'AB:CD:EF:12:34:56' must be at least 2 characters long" ) } @@ -851,13 +845,13 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "a".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "a".to_owned()); let res = options.verify(); assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name for 'ab:cd:ef:12:34:56' must be at least 2 characters long" + "interface name for 'AB:CD:EF:12:34:56' must be at least 2 characters long" ) } @@ -865,7 +859,7 @@ mod tests { fn network_interface_pinning_options_fail_on_overlong_name() { let mut options = NetworkInterfacePinningOptions::default(); options.mapping.insert( - "ab:cd:ef:12:34:56".to_owned(), + "ab:cd:ef:12:34:56".parse().unwrap(), "waytoolonginterfacename".to_owned(), ); @@ -873,7 +867,7 @@ mod tests { assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name 'waytoolonginterfacename' for 'ab:cd:ef:12:34:56' cannot be longer than 15 characters" + "interface name 'waytoolonginterfacename' for 'AB:CD:EF:12:34:56' cannot be longer than 15 characters" ) } @@ -882,10 +876,10 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "nic0".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "nic0".to_owned()); options .mapping - .insert("12:34:56:ab:cd:ef".to_owned(), "nic0".to_owned()); + .insert("12:34:56:ab:cd:ef".parse().unwrap(), "nic0".to_owned()); let res = options.verify(); assert!(res.is_err()); @@ -894,8 +888,8 @@ mod tests { // [HashMap] does not guarantee iteration order, so just check for the substrings // we expect to find assert!(err.contains("duplicate interface name mapping 'nic0' for: ")); - assert!(err.contains("12:34:56:ab:cd:ef")); - assert!(err.contains("ab:cd:ef:12:34:56")); + assert!(err.contains("12:34:56:AB:CD:EF")); + assert!(err.contains("AB:CD:EF:12:34:56")); } #[test] @@ -903,13 +897,13 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "nic-".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "nic-".to_owned()); let res = options.verify(); assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name 'nic-' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" + "interface name 'nic-' for 'AB:CD:EF:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" ) } @@ -918,24 +912,24 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "0nic".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "0nic".to_owned()); let res = options.verify(); assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name '0nic' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" + "interface name '0nic' for 'AB:CD:EF:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" ); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "_a".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "_a".to_owned()); let res = options.verify(); assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name '_a' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" + "interface name '_a' for 'AB:CD:EF:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" ); } @@ -944,21 +938,21 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "Nic0".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "Nic0".to_owned()); let res = options.verify(); assert!(res.is_ok()); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "nIc0".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "nIc0".to_owned()); let res = options.verify(); assert!(res.is_ok()); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "nic0".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "nic0".to_owned()); let res = options.verify(); assert!(res.is_ok()); @@ -969,13 +963,13 @@ mod tests { let mut options = NetworkInterfacePinningOptions::default(); options .mapping - .insert("ab:cd:ef:12:34:56".to_owned(), "12345".to_owned()); + .insert("ab:cd:ef:12:34:56".parse().unwrap(), "12345".to_owned()); let res = options.verify(); assert!(res.is_err()); assert_eq!( res.unwrap_err().to_string(), - "interface name '12345' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" + "interface name '12345' for 'AB:CD:EF:12:34:56' is invalid: name must start with a letter and contain only ascii characters, digits and underscores" ) } } diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs index b8af4e3..14882bc 100644 --- a/proxmox-installer-common/src/setup.rs +++ b/proxmox-installer-common/src/setup.rs @@ -1,4 +1,4 @@ -use serde::{Deserialize, Deserializer, Serialize, Serializer, de}; +use serde::{Deserialize, Deserializer, Serialize, Serializer, de, ser::SerializeMap}; use std::{ cmp, collections::{BTreeMap, HashMap}, @@ -18,7 +18,7 @@ use proxmox_installer_types::{ BootType, IsoInfo, ProductConfig, answer::{BtrfsCompressOption, FilesystemType, ZfsChecksumOption, ZfsCompressOption}, }; -use proxmox_network_types::Cidr; +use proxmox_network_types::{Cidr, MacAddress}; /// Paths in the ISO environment containing installer data. #[derive(Clone, Deserialize)] @@ -395,7 +395,7 @@ pub struct Interface { /// interface name cannot/should not be pinned due to being e.g. a non-physical link. pub pinned_id: Option, - pub mac: String, + pub mac: MacAddress, pub state: InterfaceState, @@ -535,9 +535,10 @@ pub struct InstallConfig { #[serde( default, skip_serializing_if = "HashMap::is_empty", - deserialize_with = "deserialize_optional_map" + deserialize_with = "deserialize_optional_map", + serialize_with = "serialize_low_level_mac_address_map" )] - pub network_interface_pin_map: HashMap, + pub network_interface_pin_map: HashMap, pub hostname: String, pub domain: String, @@ -583,3 +584,20 @@ where let map: Option> = Deserialize::deserialize(deserializer)?; Ok(map.unwrap_or_default()) } + +/// Serializes a [HashMap] using [MacAddress] as keys, normalizing MAC addresses into their +/// lower-case format as the low-level installer expects it. +fn serialize_low_level_mac_address_map( + map: &HashMap, + serializer: S, +) -> Result +where + S: Serializer, + V: Serialize, +{ + let mut serializer = serializer.serialize_map(Some(map.len()))?; + for (mac, v) in map { + serializer.serialize_entry(&mac.to_string().to_lowercase(), &v)?; + } + serializer.end() +} diff --git a/proxmox-post-hook/src/main.rs b/proxmox-post-hook/src/main.rs index 749fd0e..e50a773 100644 --- a/proxmox-post-hook/src/main.rs +++ b/proxmox-post-hook/src/main.rs @@ -286,7 +286,7 @@ mod detail { anyhow::Ok(NetworkInterfaceInfo { name: ifname.clone(), - mac: nic.mac.clone(), + mac: nic.mac, // Use the actual IP address from the low-level install config, as the runtime info // contains the original IP address from DHCP. address: is_management.then_some(config.cidr), diff --git a/proxmox-tui-installer/src/views/network.rs b/proxmox-tui-installer/src/views/network.rs index 12cef19..6401174 100644 --- a/proxmox-tui-installer/src/views/network.rs +++ b/proxmox-tui-installer/src/views/network.rs @@ -17,12 +17,12 @@ use proxmox_installer_common::{ options::{NetworkInterfacePinningOptions, NetworkOptions}, setup::{Interface, NetworkInfo}, }; -use proxmox_network_types::{fqdn::Fqdn, ip_address::Cidr}; +use proxmox_network_types::{MacAddress, fqdn::Fqdn, ip_address::Cidr}; use super::{CidrAddressEditView, FormView}; struct NetworkViewOptions { - selected_mac: String, + selected_mac: MacAddress, pinning_enabled: bool, // For UI purposes, we want to always save the mapping, to save the state // between toggling the checkbox @@ -55,14 +55,8 @@ impl NetworkOptionsView { let selected_mac = network_info .interfaces .get(&options.ifname) - .map(|iface| iface.mac.clone()) - .unwrap_or_else(|| { - ifaces - .first() - .expect("at least one network interface") - .mac - .clone() - }); + .map(|iface| iface.mac) + .unwrap_or_else(|| ifaces.first().expect("at least one network interface").mac); let options_ref = Arc::new(Mutex::new(NetworkViewOptions { selected_mac, @@ -309,7 +303,7 @@ impl NetworkOptionsView { .on_submit({ let options_ref = options_ref.clone(); move |_, iface| { - options_ref.lock().expect("unpoisoned lock").selected_mac = iface.mac.clone(); + options_ref.lock().expect("unpoisoned lock").selected_mac = iface.mac; } }); @@ -330,7 +324,7 @@ impl ViewWrapper for NetworkOptionsView { } struct InterfacePinningOptionsView { - view: ScrollView>>, + view: ScrollView>>, } impl InterfacePinningOptionsView { @@ -344,7 +338,7 @@ impl InterfacePinningOptionsView { // The low-level installer will skip them anyway. let interfaces = interfaces.iter().filter(|iface| iface.pinned_id.is_some()); - let mut form = FormView::::new(); + let mut form = FormView::::new(); for iface in interfaces { let label = format!( @@ -366,7 +360,7 @@ impl InterfacePinningOptionsView { .fixed_width(MAX_IFNAME_LEN), ); - form.add_child_with_data(&label, view, iface.mac.clone()); + form.add_child_with_data(&label, view, iface.mac); if !iface.addresses.is_empty() { for chunk in iface.addresses.chunks(2) { @@ -403,7 +397,7 @@ impl InterfacePinningOptionsView { .map(|v| v.get_inner().get_content()) .ok_or_else(|| format!("failed to retrieve pinning ID for interface {}", mac))?; - mapping.insert(mac.clone(), (*name).clone()); + mapping.insert(*mac, (*name).clone()); } let opts = NetworkInterfacePinningOptions { mapping }; @@ -414,5 +408,5 @@ impl InterfacePinningOptionsView { } impl ViewWrapper for InterfacePinningOptionsView { - cursive::wrap_impl!(self.view: ScrollView>>); + cursive::wrap_impl!(self.view: ScrollView>>); } -- 2.55.0