[pve-devel] [PATCH installer 5/6] tui: network: only display pinnable devices in pinning options dialog

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

Virtual devices such as e.g. bridges, bonds etc. otherwise also show up,
where name pinning does not make sense.

The `pinned_id` property has been made optional, thus adapt to that.

Not really an issue during bare-metal installations, as there aren't any
bridges/bonds set up yet - no changes there.
But it's a nice future-proofing and improves developing/testing
experience, as such interfaces are now suppressed).

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/options.rs    | 31 ++++++++++++--------
 proxmox-installer-common/src/setup.rs      | 33 +++++++++++++---------
 proxmox-tui-installer/src/views/network.rs | 33 +++++++++++++++-------
 3 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index a07fe58..8f3b3aa 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -595,8 +595,10 @@ impl NetworkOptions {
         {
             // we got some ipv4 connectivity, so use that
 
-            if let Some(opts) = pinning_opts {
-                this.ifname.clone_from(&iface.to_pinned(opts).name);
+            if let Some(opts) = pinning_opts
+                && let Some(pinned) = iface.to_pinned(opts)
+            {
+                this.ifname.clone_from(&pinned.name);
             } else {
                 this.ifname.clone_from(&iface.name);
             }
@@ -609,8 +611,10 @@ impl NetworkOptions {
                 && let Some(addr) = iface.addresses.iter().find(|addr| addr.is_ipv6())
             {
                 // no ipv4, but ipv6 connectivity
-                if let Some(opts) = pinning_opts {
-                    this.ifname.clone_from(&iface.to_pinned(opts).name);
+                if let Some(opts) = pinning_opts
+                    && let Some(pinned) = iface.to_pinned(opts)
+                {
+                    this.ifname.clone_from(&pinned.name);
                 } else {
                     this.ifname.clone_from(&iface.name);
                 }
@@ -627,19 +631,22 @@ impl NetworkOptions {
         if this.ifname.is_empty()
             && let Some(iface) = network.interfaces.values().min_by_key(|v| v.index)
         {
-            if let Some(opts) = pinning_opts {
-                this.ifname.clone_from(&iface.to_pinned(opts).name);
+            if let Some(opts) = pinning_opts
+                && let Some(pinned) = iface.to_pinned(opts)
+            {
+                this.ifname.clone_from(&pinned.name);
             } else {
                 this.ifname.clone_from(&iface.name);
             }
         }
 
         if let Some(ref mut opts) = this.pinning_opts {
-            // Ensure that all unique interfaces indeed have an entry in the map,
-            // as required by the low-level installer
+            // Ensure that all unique, pinnable interfaces indeed have an entry in the map, as
+            // required by the low-level installer
             for iface in network.interfaces.values() {
-                let pinned_name = iface.to_pinned(opts).name;
-                opts.mapping.entry(iface.mac.clone()).or_insert(pinned_name);
+                if let Some(pinned) = iface.to_pinned(opts) {
+                    opts.mapping.entry(iface.mac.clone()).or_insert(pinned.name);
+                }
             }
         }
 
@@ -767,7 +774,7 @@ mod tests {
             Interface {
                 name: "eth0".to_owned(),
                 index: 0,
-                pinned_id: "0".to_owned(),
+                pinned_id: Some("0".to_owned()),
                 state: InterfaceState::Up,
                 driver: "dummy".to_owned(),
                 mac: "01:23:45:67:89:ab".to_owned(),
@@ -901,7 +908,7 @@ mod tests {
             Interface {
                 name: "eth0".to_owned(),
                 index: 0,
-                pinned_id: "0".to_owned(),
+                pinned_id: Some("0".to_owned()),
                 state: InterfaceState::Up,
                 driver: "dummy".to_owned(),
                 mac: "01:23:45:67:89:ab".to_owned(),
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index c93ee30..35949f0 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -465,8 +465,9 @@ pub struct Interface {
 
     pub index: usize,
 
-    /// Sequential interface ID for pinning interface names.
-    pub pinned_id: String,
+    /// Sequential interface ID for pinning interface names. If unset, the
+    /// interface name cannot/should not be pinned due to being e.g. a non-physical link.
+    pub pinned_id: Option<String>,
 
     pub mac: String,
 
@@ -492,19 +493,23 @@ impl Interface {
         )
     }
 
-    pub fn to_pinned(&self, options: &NetworkInterfacePinningOptions) -> Self {
-        let mut this = self.clone();
-        this.name = options
-            .mapping
-            .get(&this.mac)
-            .unwrap_or(&format!(
-                "{}{}",
-                NetworkInterfacePinningOptions::DEFAULT_PREFIX,
-                this.pinned_id
-            ))
-            .clone();
+    pub fn to_pinned(&self, options: &NetworkInterfacePinningOptions) -> Option<Self> {
+        if let Some(pinned_id) = &self.pinned_id {
+            let mut this = self.clone();
+            this.name = options
+                .mapping
+                .get(&this.mac)
+                .unwrap_or(&format!(
+                    "{}{}",
+                    NetworkInterfacePinningOptions::DEFAULT_PREFIX,
+                    pinned_id
+                ))
+                .clone();
 
-        this
+            Some(this)
+        } else {
+            None
+        }
     }
 }
 
diff --git a/proxmox-tui-installer/src/views/network.rs b/proxmox-tui-installer/src/views/network.rs
index 4388c7d..970c353 100644
--- a/proxmox-tui-installer/src/views/network.rs
+++ b/proxmox-tui-installer/src/views/network.rs
@@ -189,9 +189,12 @@ impl NetworkOptionsView {
             .map(|opt| opt.pinning_enabled.then_some(opt.pinning_options.clone()))
             .map_err(|err| err.to_string())?;
 
-        let ifname = match &pinning_opts {
-            Some(opts) => iface.to_pinned(opts).name,
-            None => iface.name,
+        let ifname = if let Some(opts) = &pinning_opts
+            && let Some(pinned) = iface.to_pinned(opts)
+        {
+            pinned.name
+        } else {
+            iface.name
         };
 
         if address.addr().is_ipv4() != gateway.is_ipv4() {
@@ -291,13 +294,13 @@ impl NetworkOptionsView {
         let ifnames = ifaces
             .iter()
             .map(|iface| {
-                let iface = if options.pinning_enabled {
-                    &iface.to_pinned(&options.pinning_options)
+                if options.pinning_enabled
+                    && let Some(pinned) = iface.to_pinned(&options.pinning_options)
+                {
+                    (pinned.render(), pinned.clone())
                 } else {
-                    iface
-                };
-
-                (iface.render(), iface.clone())
+                    (iface.render(), (*iface).clone())
+                }
             })
             .collect::<Vec<(String, Interface)>>();
 
@@ -337,6 +340,11 @@ impl InterfacePinningOptionsView {
     fn new(interfaces: &[&Interface], options_ref: NetworkViewOptionsRef) -> Self {
         let options = options_ref.lock().expect("unpoisoned lock");
 
+        // Filter out all non-physical links, as it does not make sense to pin their names
+        // in this way.
+        // 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();
 
         for iface in interfaces {
@@ -349,7 +357,12 @@ impl InterfacePinningOptionsView {
                 .child(DummyView.full_width()) // right align below form elements
                 .child(
                     EditView::new()
-                        .content(iface.to_pinned(&options.pinning_options).name)
+                        .content(
+                            iface
+                                .to_pinned(&options.pinning_options)
+                                .expect("always pinnable interface")
+                                .name,
+                        )
                         .max_content_width(MAX_IFNAME_LEN)
                         .fixed_width(MAX_IFNAME_LEN),
                 );
-- 
2.51.0



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