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

[Christoph Heiss]

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

By making the `pinned_id` property on our internal network device
structure optional, any other code can easily determine whether an
interface is pinnable or not.

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 at proxmox.com>
---
 Proxmox/Install.pm        |  9 +++------
 Proxmox/Install/RunEnv.pm | 12 +++++++++---
 Proxmox/Sys/Net.pm        | 18 ------------------
 proxinstall               | 10 ++++++++--
 4 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 1704ec4..bfc1769 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -1108,15 +1108,12 @@ sub extract_data {
             mkdir "$targetdir/usr/local/lib/systemd", 0755;
             mkdir "$targetdir/usr/local/lib/systemd/network", 0755;
 
-            my $ip_links = Proxmox::Sys::Net::ip_link_details();
-
             for my $ifname (sort keys $run_env->{network}->{interfaces}->%*) {
-                next
-                    if !Proxmox::Sys::Net::ip_link_is_physical($ip_links->{$ifname})
-                    || Proxmox::Sys::Net::iface_is_vf($ifname);
-
                 my $if = $run_env->{network}->{interfaces}->{$ifname};
 
+                # skip unpinnable interfaces, i.e. virtual links
+                next if !defined($if->{pinned_id});
+
                 if (!defined($netif_pin_map->{ $if->{mac} })) {
                     # each installer frontend must ensure that either
                     #   - pinning is disabled by never setting the map or
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 40a9621..fd3f74c 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -87,7 +87,7 @@ my sub query_netdevs : prototype() {
     my $default;
 
     # FIXME: not the same as the battle proven way we used in the installer for years?
-    my $interfaces = fromjs(qx/ip --json address show/);
+    my $interfaces = fromjs(qx/ip --details --json address show/);
 
     my $pinned_counter = 0;
     for my $if (@$interfaces) {
@@ -122,14 +122,20 @@ my sub query_netdevs : prototype() {
         $ifs->{$name} = {
             index => $index,
             name => $name,
-            pinned_id => "${pinned_counter}",
             mac => $mac,
             state => uc($state),
             driver => $driver,
         };
         $ifs->{$name}->{addresses} = \@valid_addrs if @valid_addrs;
 
-        $pinned_counter++;
+        # only set the `pinned_id` property if the interface actually can be pinned,
+        # i.e. is a physical link
+        my $is_pinnable =
+            Proxmox::Sys::Net::ip_link_is_physical($if) && !Proxmox::Sys::Net::iface_is_vf($name);
+        if ($is_pinnable) {
+            $ifs->{$name}->{pinned_id} = "${pinned_counter}";
+            $pinned_counter++;
+        }
     }
 
     return $ifs;
diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index 016a7f8..f6a0940 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -153,24 +153,6 @@ sub iface_is_vf {
     return -l "/sys/class/net/$iface_name/device/physfn";
 }
 
-# Duplicated from pve-common/src/PVE/Network.pm for now
-sub ip_link_details {
-    my $link_json = '';
-
-    Proxmox::Sys::Command::run_command(
-        ['ip', '-details', '-json', 'link', 'show'],
-        sub {
-            $link_json .= shift;
-            return;
-        },
-    );
-
-    my $links = JSON::decode_json($link_json);
-    my %ip_links = map { $_->{ifname} => $_ } $links->@*;
-
-    return \%ip_links;
-}
-
 # Duplicated from pve-common/src/PVE/Network.pm from now
 sub ip_link_is_physical {
     my ($ip_link) = @_;
diff --git a/proxinstall b/proxinstall
index e3ea22e..e076c25 100755
--- a/proxinstall
+++ b/proxinstall
@@ -374,9 +374,15 @@ my sub create_network_interface_pin_view {
 
     my $inputs = {};
     my $row = 0;
+
     for my $ifname (sort keys $interfaces->%*) {
         my $iface = $interfaces->{$ifname};
 
+        # exclude all non-pinnable interfaces, i.e. non-physical links, which won't have their
+        # `pinned_id` set.
+        # It does not make sense to pin those names, and the low-level installer will also skip them
+        next if !defined($iface->{pinned_id});
+
         my $name = $mapping->{ $iface->{mac} };
         my $label_text = "$iface->{mac} ($ifname, $iface->{driver}, $iface->{state})";
 
@@ -463,7 +469,7 @@ sub create_ipconf_view {
 
             my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' ';
             my $name =
-                $gtk_state->{network_pinning_enabled}
+                $gtk_state->{network_pinning_enabled} && defined($mapping->{ $iface->{mac} })
                 ? $mapping->{ $iface->{mac} }
                 : $iface->{name};
 
@@ -1920,7 +1926,7 @@ if (!$initial_error && (scalar keys $run_env->{ipconf}->{ifaces}->%* == 0)) {
 
     # pre-fill the name mapping before starting
     my %mapping = map {
-        $_->{mac} => DEFAULT_PIN_PREFIX . $_->{pinned_id}
+        defined($_->{pinned_id}) ? ($_->{mac} => DEFAULT_PIN_PREFIX . $_->{pinned_id}) : ()
     } values $run_env->{network}->{interfaces}->%*;
     Proxmox::Install::Config::set_network_interface_pin_map(\%mapping);
 }
-- 
2.51.0