[PATCH installer 04/15] tree-wide: use `MacAddress` type instead of string for MAC addresses

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

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 <c.heiss@proxmox.com>
---
 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<String, String>,
+    pub mapping: HashMap<MacAddress, String>,
 }
 
 impl NetworkInterfacePinningOptions {
@@ -341,7 +341,7 @@ impl NetworkInterfacePinningOptions {
                 .unwrap()
         });
 
-        let mut reverse_mapping = HashMap::<String, String>::new();
+        let mut reverse_mapping = HashMap::<String, MacAddress>::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<String>,
 
-    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<String, String>,
+    pub network_interface_pin_map: HashMap<MacAddress, String>,
 
     pub hostname: String,
     pub domain: String,
@@ -583,3 +584,20 @@ where
     let map: Option<HashMap<K, V>> = 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<S, V>(
+    map: &HashMap<MacAddress, V>,
+    serializer: S,
+) -> Result<S::Ok, S::Error>
+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<NamedView<FormView<String>>>,
+    view: ScrollView<NamedView<FormView<MacAddress>>>,
 }
 
 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::<String>::new();
+        let mut form = FormView::<MacAddress>::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<NamedView<FormView<String>>>);
+    cursive::wrap_impl!(self.view: ScrollView<NamedView<FormView<MacAddress>>>);
 }
-- 
2.55.0