[pve-devel] [PATCH installer 0/6] interface name pinning followups

[pve-devel] [PATCH installer 0/6] interface name pinning followups

[Christoph Heiss]

Some small followups for the initial name pinning support [0].

First and foremost makes the interface name checks a bit stricter, to
comply with our `pve-iface` schema - as reported by Stoiko [1]. Thanks!

Patch #4 & #5 exclude non-physical interfaces from being shown in the
pinning dialog in the GUI and TUI, respectively. While this does change
anything for normal bare-metal installation, it is a) a bit of
future-proofing and b) improves developer/testing experience a bit, as
these interfaces should never be available for pinning anyway.

[0] https://lore.proxmox.com/pve-devel/20251111140014.1443471-1-c.heiss@proxmox.com/
[1] https://lore.proxmox.com/pve-devel/20251111181216.3c1ca6a0@rosa.proxmox.com/

Diffstat
========

Christoph Heiss (6):
  sys: net: pinning: make interface name checks stricter
  common: pinning: make interface name checks stricter
  test: validate-link-pin-map: give all tests a name
  gui: network: only display pinnable devices in pinning options dialog
  tui: network: only display pinnable devices in pinning options dialog
  tree-wide: run cargo fmt

 Proxmox/Install.pm                            |  9 +--
 Proxmox/Install/RunEnv.pm                     | 12 ++-
 Proxmox/Sys/Net.pm                            | 35 +++------
 proxinstall                                   | 13 +++-
 .../src/bin/proxmox-auto-installer.rs         |  7 +-
 proxmox-auto-installer/src/utils.rs           | 15 ++--
 .../src/fetch_plugins/http.rs                 |  7 +-
 .../src/fetch_plugins/partition.rs            |  7 +-
 proxmox-installer-common/src/lib.rs           |  7 +-
 proxmox-installer-common/src/options.rs       | 77 ++++++++++++++-----
 proxmox-installer-common/src/setup.rs         | 33 ++++----
 proxmox-tui-installer/src/main.rs             |  7 +-
 proxmox-tui-installer/src/views/network.rs    | 33 +++++---
 test/validate-link-pin-map.pl                 | 34 ++++++--
 14 files changed, 189 insertions(+), 107 deletions(-)

-- 
2.51.0



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

[pve-devel] [PATCH installer 1/6] sys: net: pinning: make interface name checks stricter

[Christoph Heiss]

According to our `pve-iface` schema, names must be at least two
characters long and start with a (latin) letter.

Reported-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Net.pm            | 17 +++++++++++------
 proxinstall                   |  3 +--
 test/validate-link-pin-map.pl | 23 +++++++++++++++++++++--
 3 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index 991f723..016a7f8 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -13,13 +13,16 @@ our @EXPORT_OK = qw(
     parse_ip_mask
     parse_fqdn
     validate_link_pin_map
+    MIN_IFNAME_LEN
     MAX_IFNAME_LEN
     DEFAULT_PIN_PREFIX
 );
 
-# Maximum length of the (primary) name of a network interface
-# IFNAMSIZ - 1 to account for NUL byte
 use constant {
+    # As dictated by the `pve-iface` schema.
+    MIN_IFNAME_LEN => 2,
+    # Maximum length of the (primary) name of a network interface.
+    # IFNAMSIZ - 1 to account for NUL byte
     MAX_IFNAME_LEN => 15,
     DEFAULT_PIN_PREFIX => 'nic',
 };
@@ -338,8 +341,10 @@ sub validate_link_pin_map : prototype($) {
     my $reverse_mapping = {};
 
     while (my ($mac, $name) = each %$mapping) {
-        if (!defined($name) || $name eq '') {
-            die "interface name for '$mac' cannot be empty\n";
+        if (!defined($name) || length($name) < MIN_IFNAME_LEN) {
+            die "interface name for '$mac' must be at least "
+                . MIN_IFNAME_LEN
+                . " characters long\n";
         }
 
         if (length($name) > MAX_IFNAME_LEN) {
@@ -353,8 +358,8 @@ sub validate_link_pin_map : prototype($) {
                 . "name must not be fully numeric\n";
         }
 
-        if ($name =~ m/^[0-9]/) {
-            die "interface name '$name' for '$mac' is invalid: name must not start with a number\n";
+        if ($name !~ m/^[a-z]/) {
+            die "interface name '$name' for '$mac' is invalid: name must start with a letter\n";
         }
 
         if ($name !~ m/^[a-zA-Z_][a-zA-Z0-9_]*$/) {
diff --git a/proxinstall b/proxinstall
index 49dd796..e3ea22e 100755
--- a/proxinstall
+++ b/proxinstall
@@ -37,8 +37,7 @@ use Proxmox::Sys;
 use Proxmox::Sys::Block qw(get_cached_disks);
 use Proxmox::Sys::Command qw(syscmd);
 use Proxmox::Sys::File qw(file_read_all file_write_all);
-use Proxmox::Sys::Net
-    qw(parse_ip_address parse_ip_mask validate_link_pin_map MAX_IFNAME_LEN DEFAULT_PIN_PREFIX);
+use Proxmox::Sys::Net qw(parse_ip_address parse_ip_mask validate_link_pin_map DEFAULT_PIN_PREFIX);
 use Proxmox::UI;
 
 my $step_number = 0; # Init number for global function list
diff --git a/test/validate-link-pin-map.pl b/test/validate-link-pin-map.pl
index 6386700..37e8387 100755
--- a/test/validate-link-pin-map.pl
+++ b/test/validate-link-pin-map.pl
@@ -8,7 +8,18 @@ use Test::More;
 use Proxmox::Sys::Net qw(validate_link_pin_map);
 
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '' }) };
-is($@, "interface name for 'ab:cd:ef:12:34:56' cannot be empty\n");
+is(
+    $@,
+    "interface name for 'ab:cd:ef:12:34:56' must be at least 2 characters long\n",
+    "empty name is rejected",
+);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'a' }) };
+is(
+    $@,
+    "interface name for 'ab:cd:ef:12:34:56' must be at least 2 characters long\n",
+    "1 character name is rejected",
+);
 
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'waytoolonginterfacename' }) };
 is(
@@ -30,7 +41,15 @@ is(
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '0nic' }) };
 is(
     $@,
-    "interface name '0nic' for 'ab:cd:ef:12:34:56' is invalid: name must not start with a number\n",
+    "interface name '0nic' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter\n",
+    "name starting with number is rejected",
+);
+
+eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '_a' }) };
+is(
+    $@,
+    "interface name '_a' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter\n",
+    "name starting with underscore is rejected",
 );
 
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '12345' }) };
-- 
2.51.0



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

[pve-devel] [PATCH installer 2/6] common: pinning: make interface name checks stricter

[Christoph Heiss]

According to our `pve-iface` schema, names must be at least two
characters long and start with a (latin) letter.

Reported-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/lib.rs     |  7 ++--
 proxmox-installer-common/src/options.rs | 46 ++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/proxmox-installer-common/src/lib.rs b/proxmox-installer-common/src/lib.rs
index a85d5f8..b380f1c 100644
--- a/proxmox-installer-common/src/lib.rs
+++ b/proxmox-installer-common/src/lib.rs
@@ -11,8 +11,11 @@ pub mod http;
 pub mod cli;
 
 pub mod net {
-    /// Maximum length of the (primary) name of a network interface
-    pub const MAX_IFNAME_LEN: usize = 15; // IFNAMSIZ - 1 to account for NUL byte
+    /// As dictated by the `pve-iface` schema.
+    pub const MIN_IFNAME_LEN: usize = 2;
+    /// Maximum length of the (primary) name of a network interface.
+    /// IFNAMSIZ - 1 to account for NUL byte
+    pub const MAX_IFNAME_LEN: usize = 15;
 }
 
 pub const RUNTIME_DIR: &str = "/run/proxmox-installer";
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 0d72f54..a07fe58 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -8,7 +8,7 @@ use std::sync::OnceLock;
 use std::{cmp, fmt};
 
 use crate::disk_checks::check_raid_min_disks;
-use crate::net::MAX_IFNAME_LEN;
+use crate::net::{MAX_IFNAME_LEN, MIN_IFNAME_LEN};
 use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
 use crate::utils::{CidrAddress, Fqdn};
 
@@ -504,8 +504,10 @@ impl NetworkInterfacePinningOptions {
     pub fn verify(&self) -> Result<()> {
         let mut reverse_mapping = HashMap::<String, String>::new();
         for (mac, name) in self.mapping.iter() {
-            if name.is_empty() {
-                bail!("interface name for '{mac}' cannot be empty");
+            if name.len() < MIN_IFNAME_LEN {
+                bail!(
+                    "interface name for '{mac}' must be at least {MIN_IFNAME_LEN} characters long"
+                );
             }
 
             if name.len() > MAX_IFNAME_LEN {
@@ -522,9 +524,9 @@ impl NetworkInterfacePinningOptions {
             }
 
             // Mimicking the `pve-iface` schema verification
-            if name.starts_with(|c: char| c.is_ascii_digit()) {
+            if !name.starts_with(|c: char| c.is_ascii_alphabetic()) {
                 bail!(
-                    "interface name '{name}' for '{mac}' is invalid: name must not start with a number"
+                    "interface name '{name}' for '{mac}' is invalid: name must start with a letter"
                 );
             }
 
@@ -943,7 +945,22 @@ mod tests {
         assert!(res.is_err());
         assert_eq!(
             res.unwrap_err().to_string(),
-            "interface name for 'ab:cd:ef:12:34:56' cannot be empty"
+            "interface name for 'ab:cd:ef:12:34:56' must be at least 2 characters long"
+        )
+    }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_too_short_name() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), "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"
         )
     }
 
@@ -1000,7 +1017,7 @@ mod tests {
     }
 
     #[test]
-    fn network_interface_pinning_options_fail_on_name_starting_with_number() {
+    fn network_interface_pinning_options_fail_on_nonletter_first_char() {
         let mut options = NetworkInterfacePinningOptions::default();
         options
             .mapping
@@ -1010,8 +1027,19 @@ mod tests {
         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 not start with a number"
-        )
+            "interface name '0nic' for 'ab:cd:ef:12:34:56' is invalid: name must start with a letter"
+        );
+
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), "_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"
+        );
     }
 
     #[test]
-- 
2.51.0



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

[pve-devel] [PATCH installer 3/6] test: validate-link-pin-map: give all tests a name

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 test/validate-link-pin-map.pl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/test/validate-link-pin-map.pl b/test/validate-link-pin-map.pl
index 37e8387..3a95aab 100755
--- a/test/validate-link-pin-map.pl
+++ b/test/validate-link-pin-map.pl
@@ -25,17 +25,21 @@ eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'waytoolonginterfacename'
 is(
     $@,
     "interface name 'waytoolonginterfacename' for 'ab:cd:ef:12:34:56' cannot be longer than 15 characters\n",
+    "too long name is rejected",
 );
 
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'nic0', '12:34:56:ab:cd:ef' => 'nic0' }) };
-like($@, qr/^duplicate interface name mapping 'nic0' for: /);
-like($@, qr/ab:cd:ef:12:34:56/);
-like($@, qr/12:34:56:ab:cd:ef/);
+like(
+    $@, qr/^duplicate interface name mapping 'nic0' for: /, "duplicate assignment is rejected",
+);
+like($@, qr/ab:cd:ef:12:34:56/, "duplicate error message contains expected mac address");
+like($@, qr/12:34:56:ab:cd:ef/, "duplicate error message contains expected mac address");
 
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => 'nic-' }) };
 is(
     $@,
     "interface name 'nic-' for 'ab:cd:ef:12:34:56' is invalid: name must only consist of alphanumeric characters and underscores\n",
+    "name with dash is rejected",
 );
 
 eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '0nic' }) };
@@ -56,6 +60,7 @@ eval { validate_link_pin_map({ 'ab:cd:ef:12:34:56' => '12345' }) };
 is(
     $@,
     "interface name '12345' for 'ab:cd:ef:12:34:56' is invalid: name must not be fully numeric\n",
+    "fully numeric name is rejected",
 );
 
 done_testing();
-- 
2.51.0



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

[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@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



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

[pve-devel] [PATCH installer 5/6] tui: 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.

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

[pve-devel] [PATCH installer 6/6] tree-wide: run cargo fmt

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 .../src/bin/proxmox-auto-installer.rs             |  7 ++++---
 proxmox-auto-installer/src/utils.rs               | 15 +++++++++------
 proxmox-fetch-answer/src/fetch_plugins/http.rs    |  7 ++++---
 .../src/fetch_plugins/partition.rs                |  7 ++++---
 proxmox-tui-installer/src/main.rs                 |  7 ++++---
 5 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index 467ef1b..7614fbb 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -123,9 +123,10 @@ fn main() -> ExitCode {
     };
 
     if answer.global.reboot_on_error
-        && let Err(err) = File::create("/run/proxmox-reboot-on-error") {
-            error!("failed to create reboot-on-error flag-file: {err}");
-        }
+        && let Err(err) = File::create("/run/proxmox-reboot-on-error")
+    {
+        error!("failed to create reboot-on-error flag-file: {err}");
+    }
 
     if answer.global.reboot_mode == RebootMode::PowerOff {
         if let Err(err) = File::create("/run/proxmox-poweroff-after-install") {
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index f5c3f70..884a08e 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -426,9 +426,10 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
     }
 
     if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options
-        && let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize) {
-            check_swapsize(swapsize, hdsize)?;
-        }
+        && let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize)
+    {
+        check_swapsize(swapsize, hdsize)?;
+    }
 
     Ok(())
 }
@@ -437,9 +438,11 @@ pub fn verify_first_boot_settings(answer: &Answer) -> Result<()> {
     info!("Verifying first boot settings");
 
     if let Some(first_boot) = &answer.first_boot
-        && first_boot.source == FirstBootHookSourceMode::FromUrl && first_boot.url.is_none() {
-            bail!("first-boot executable source set to URL, but none specified!");
-        }
+        && first_boot.source == FirstBootHookSourceMode::FromUrl
+        && first_boot.url.is_none()
+    {
+        bail!("first-boot executable source set to URL, but none specified!");
+    }
 
     Ok(())
 }
diff --git a/proxmox-fetch-answer/src/fetch_plugins/http.rs b/proxmox-fetch-answer/src/fetch_plugins/http.rs
index d6331d8..e2fd633 100644
--- a/proxmox-fetch-answer/src/fetch_plugins/http.rs
+++ b/proxmox-fetch-answer/src/fetch_plugins/http.rs
@@ -140,9 +140,10 @@ impl FetchFromHTTP {
         info!("Retrieving default search domain.");
         for line in read_to_string("/etc/resolv.conf")?.lines() {
             if let Some((key, value)) = line.split_once(' ')
-                && key == "search" {
-                    return Ok(value.trim().into());
-                }
+                && key == "search"
+            {
+                return Ok(value.trim().into());
+            }
         }
         bail!("Could not find search domain in resolv.conf.");
     }
diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
index f952513..3eac301 100644
--- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
+++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
@@ -123,9 +123,10 @@ fn check_if_mounted(target_path: &str) -> Result<bool> {
     let mounts = fs::read_to_string("/proc/mounts")?;
     for line in mounts.lines() {
         if let Some(mp) = line.split(' ').nth(1)
-            && mp == target_path {
-                return Ok(true);
-            }
+            && mp == target_path
+        {
+            return Ok(true);
+        }
     }
     Ok(false)
 }
diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 05e6a8e..d2fd3d8 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -197,9 +197,10 @@ fn installer_setup_late(siv: &mut Cursive) {
     if !state.in_test_mode {
         let kmap_id = &state.options.timezone.kb_layout;
         if let Some(kmap) = state.locales.kmap.get(kmap_id)
-            && let Err(err) = system::set_keyboard_layout(kmap) {
-                display_setup_warning(siv, &format!("Failed to apply keyboard layout: {err}"));
-            }
+            && let Err(err) = system::set_keyboard_layout(kmap)
+        {
+            display_setup_warning(siv, &format!("Failed to apply keyboard layout: {err}"));
+        }
     }
 
     if state.runtime_info.total_memory < 1024 {
-- 
2.51.0



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

[pve-devel] applied: [PATCH installer 0/6] interface name pinning followups

[Thomas Lamprecht]

On Thu, 13 Nov 2025 14:49:48 +0100, Christoph Heiss wrote:
> Some small followups for the initial name pinning support [0].
> 
> First and foremost makes the interface name checks a bit stricter, to
> comply with our `pve-iface` schema - as reported by Stoiko [1]. Thanks!
> 
> Patch #4 & #5 exclude non-physical interfaces from being shown in the
> pinning dialog in the GUI and TUI, respectively. While this does change
> anything for normal bare-metal installation, it is a) a bit of
> future-proofing and b) improves developer/testing experience a bit, as
> these interfaces should never be available for pinning anyway.
> 
> [...]

Applied, thanks!

[1/6] sys: net: pinning: make interface name checks stricter
      commit: 2033a3576fae1f684024710541508f16b04d72de
[2/6] common: pinning: make interface name checks stricter
      commit: aa0ddcdbff34f8bd3d16e017df3ef160e2e1cdd5
[3/6] test: validate-link-pin-map: give all tests a name
      commit: 24f69569adcec258edc74190b111e71858e3ca4d
[4/6] gui: network: only display pinnable devices in pinning options dialog
      commit: 3520f0dc1941e06dd4b1192007a3e0e97c7c8f0e
[5/6] tui: network: only display pinnable devices in pinning options dialog
      commit: 32b2ea3bb230c22008f7f3af6c9ab7c433069561
[6/6] tree-wide: run cargo fmt
      commit: d505f71b3c5da129880ca9b15e95e0946d9933cb


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