From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id CBF5D1FF17A for ; Tue, 11 Nov 2025 17:48:59 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 30D181100D; Tue, 11 Nov 2025 17:49:44 +0100 (CET) Date: Tue, 11 Nov 2025 17:49:00 +0100 From: Stoiko Ivanov To: Christoph Heiss Message-ID: <20251111174900.16fc6e74@rosa.proxmox.com> In-Reply-To: <20251111140014.1443471-16-c.heiss@proxmox.com> References: <20251111140014.1443471-1-c.heiss@proxmox.com> <20251111140014.1443471-16-c.heiss@proxmox.com> X-Mailer: Claws Mail 4.3.1 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762879717277 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.072 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [parse-kernel-cmdline.pl, validate-link-pin-map.pl, perl.org, net.pm] Subject: Re: [pve-devel] [PATCH installer v3 15/15] gui: add support for pinning network interface names X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" one cosmetic nit (if even that) inline: On Tue, 11 Nov 2025 15:00:05 +0100 Christoph Heiss wrote: > Adds an additional checkbox and option button in the network panel, the > latter triggering a dialog for setting custom names per network > interface present on the system. > > Pinning is enabled by default. > > Each pinned network interface name defaults to `nicN`, where N is the > pinned ID from the low-level installer. > > Signed-off-by: Christoph Heiss > --- > Changes v2 -> v3: > * moved pinning map validation into own subroutine in > Proxmox::Sys::Net > * added unit tests for the above, taken from the rust implementation > > Changes v1 -> v2: > * added better validation for interface names, now according to our > `pve-iface` json schema > * implemented gtk-specific suggestions made by Maximilano > * run `make tidy` > * fixed error message dialog getting wrongly z-ordered > > Proxmox/Sys/Net.pm | 62 ++++++++++- > proxinstall | 189 ++++++++++++++++++++++++++++++---- > test/Makefile | 7 +- > test/validate-link-pin-map.pl | 42 ++++++++ > 4 files changed, 278 insertions(+), 22 deletions(-) > create mode 100755 test/validate-link-pin-map.pl > > diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm > index 2183d27..991f723 100644 > --- a/Proxmox/Sys/Net.pm > +++ b/Proxmox/Sys/Net.pm > @@ -8,7 +8,21 @@ use Proxmox::Sys::Udev; > use JSON qw(); > > use base qw(Exporter); > -our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn); > +our @EXPORT_OK = qw( > + parse_ip_address > + parse_ip_mask > + parse_fqdn > + validate_link_pin_map > + 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 { > + MAX_IFNAME_LEN => 15, > + DEFAULT_PIN_PREFIX => 'nic', > +}; > > our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{,61}?[a-zA-Z0-9])?)"; > our $FQDN_RE = "(?:${HOSTNAME_RE}\\.)*${HOSTNAME_RE}"; > @@ -310,4 +324,50 @@ sub parse_fqdn : prototype($) { > die "Hostname does not look like a fully qualified domain name\n"; > } > > +# Does some basic checks on a link name mapping. > +# Takes a hashref mapping mac => name. > +# > +# Includes checks for: > +# - empty interface names > +# - overlong interface names > +# - duplicate interface names > +# - only contains ASCII alphanumeric characters and underscore, as enforced by > +# our `pve-iface` json schema. > +sub validate_link_pin_map : prototype($) { > + my ($mapping) = @_; > + my $reverse_mapping = {}; > + > + while (my ($mac, $name) = each %$mapping) { stumbled over this - as it's not too common in our perl code-base later realized we use it in the installer quite a bit (since 2018: 2e33c3f ("implemented acknowledgement screen")) then got a bit side-tracked - as of perl 5.36 this can also be solved with a multi-value for loop - see: https://perldoc.perl.org/functions/each but it's ok as is! > + if (!defined($name) || $name eq '') { > + die "interface name for '$mac' cannot be empty\n"; > + } > + > + if (length($name) > MAX_IFNAME_LEN) { > + die "interface name '$name' for '$mac' cannot be longer than " > + . MAX_IFNAME_LEN > + . " characters\n"; > + } > + > + if ($name =~ m/^[0-9]+$/) { > + die "interface name '$name' for '$mac' is invalid: " > + . "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-zA-Z_][a-zA-Z0-9_]*$/) { > + die "interface name '$name' for '$mac' is invalid: " > + . "name must only consist of alphanumeric characters and underscores\n"; > + } > + > + if ($reverse_mapping->{$name}) { > + die "duplicate interface name mapping '$name' for: $mac, $reverse_mapping->{$name}\n"; > + } > + > + $reverse_mapping->{$name} = $mac; > + } > +} > + > 1; > diff --git a/proxinstall b/proxinstall > index 5ba65fa..49dd796 100755 > --- a/proxinstall > +++ b/proxinstall > @@ -37,7 +37,8 @@ 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); > +use Proxmox::Sys::Net > + qw(parse_ip_address parse_ip_mask validate_link_pin_map MAX_IFNAME_LEN DEFAULT_PIN_PREFIX); > use Proxmox::UI; > > my $step_number = 0; # Init number for global function list > @@ -340,11 +341,92 @@ my $create_basic_grid = sub { > return $grid; > }; > > +my sub create_network_interface_pin_view { > + my ($done_cb) = @_; > + > + my $dialog = Gtk3::Dialog->new(); > + $dialog->set_title('Interface Name Pinning Options'); > + $dialog->add_button('_OK', 'ok'); > + > + my $content = $dialog->get_content_area(); > + > + my $hbox = Gtk3::Box->new('horizontal', 0); > + $content->pack_start($hbox, 1, 1, 5); > + > + my $grid = Gtk3::Grid->new(); > + $grid->set_column_spacing(10); > + $grid->set_row_spacing(10); > + > + # make the list scrollable, in case there are lots of interfaces > + my $scrolled_window = Gtk3::ScrolledWindow->new(); > + $scrolled_window->set_hexpand(1); > + $scrolled_window->set_propagate_natural_height(1); > + > + $scrolled_window->add($grid); > + $scrolled_window->set_policy('never', 'automatic'); > + $scrolled_window->set_min_content_height(200); > + $scrolled_window->set_margin_start(10); > + $scrolled_window->set_margin_end(10); > + > + $hbox->pack_start($scrolled_window, 1, 0, 5); > + > + my $interfaces = Proxmox::Install::RunEnv::get()->{network}->{interfaces}; > + my $mapping = Proxmox::Install::Config::get_network_interface_pin_map(); > + > + my $inputs = {}; > + my $row = 0; > + for my $ifname (sort keys $interfaces->%*) { > + my $iface = $interfaces->{$ifname}; > + > + my $name = $mapping->{ $iface->{mac} }; > + my $label_text = "$iface->{mac} ($ifname, $iface->{driver}, $iface->{state})"; > + > + # if the interface has addresses assigned through DHCP, show them for > + # reference > + if (defined($iface->{addresses})) { > + $label_text .= > + "\n " . join(', ', map { "$_->{address}/$_->{prefix}" } @{ $iface->{addresses} }); > + } > + > + my ($label, $input) = create_text_input($name, $label_text); > + $label->set_xalign(0.); > + > + $grid->attach($label, 0, $row, 1, 1); > + $grid->attach($input, 1, $row, 1, 1); > + $row++; > + > + $inputs->{ $iface->{mac} } = $input; > + } > + > + $hbox->show_all(); > + > + $dialog->signal_connect( > + response => sub { > + my %new_mapping = map { $_ => $inputs->{$_}->get_text() } keys %$inputs; > + > + eval { validate_link_pin_map(\%new_mapping); }; > + if ($@) { > + Proxmox::UI::message($@, $dialog); > + return; > + } > + > + Proxmox::Install::Config::set_network_interface_pin_map(\%new_mapping); > + $dialog->destroy(); > + $done_cb->(); > + }, > + ); > + > + $dialog->present(); > +} > + > sub create_ipconf_view { > > cleanup_view(); > Proxmox::UI::display_html('ipconf.htm'); > > + my $run_env = Proxmox::Install::RunEnv::get(); > + my $ipconf = $run_env->{ipconf}; > + > my $grid = &$create_basic_grid(); > $grid->set_row_spacing(10); > $grid->set_column_spacing(10); > @@ -355,7 +437,7 @@ sub create_ipconf_view { > > my ($cidr_label, $cidr_box, $ipconf_entry_addr, $ipconf_entry_mask) = create_cidr_inputs($cidr); > > - my $device_model = Gtk3::ListStore->new('Glib::String', 'Glib::String'); > + my $device_model = Gtk3::ListStore->new('Glib::String', 'Glib::String', 'Glib::String'); > my $device_cb = Gtk3::ComboBox->new_with_model($device_model); > $device_cb->set_active(0); > $device_cb->set_visible(1); > @@ -369,19 +451,62 @@ sub create_ipconf_view { > $device_cb->pack_start($cell, 0); > $device_cb->add_attribute($cell, 'text', 1); > > - my $get_device_desc = sub { > - my $iface = shift; > - return "$iface->{name} - $iface->{mac} ($iface->{driver})"; > + my $refresh_device_cb = sub { > + # clear all entries and re-add them with their new names > + my $active = $device_cb->get_active(); > + $device_model->clear(); > + > + my $mapping = Proxmox::Install::Config::get_network_interface_pin_map(); > + my $i = 0; > + for my $index (sort keys $ipconf->{ifaces}->%*) { > + my $iface = $ipconf->{ifaces}->{$index}; > + my $iter = $device_model->append(); > + > + my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' '; > + my $name = > + $gtk_state->{network_pinning_enabled} > + ? $mapping->{ $iface->{mac} } > + : $iface->{name}; > + > + $device_model->set( > + $iter, > + 0 => $symbol, > + 1 => "$name - $iface->{mac} ($iface->{driver})", > + ); > + $i++; > + } > + > + # re-set the currently active entry to keep the users selection > + $device_cb->set_active($active); > }; > > - my $run_env = Proxmox::Install::RunEnv::get(); > - my $ipconf = $run_env->{ipconf}; > + my $name_pin_opts_button = Gtk3::Button->new('Options'); > + $name_pin_opts_button->set_sensitive($gtk_state->{network_pinning_enabled}); > + $name_pin_opts_button->signal_connect( > + clicked => sub { > + create_network_interface_pin_view($refresh_device_cb); > + }, > + ); > + > + my $name_pin_checkbox = Gtk3::CheckButton->new('Pin network interface names'); > + $name_pin_checkbox->set_active($gtk_state->{network_pinning_enabled}); > + $name_pin_checkbox->signal_connect( > + toggled => sub { > + $name_pin_opts_button->set_sensitive(!!$name_pin_checkbox->get_active()); > + $gtk_state->{network_pinning_enabled} = !!$name_pin_checkbox->get_active(); > + $refresh_device_cb->(); > + }, > + ); > > my ($device_active_map, $device_active_reverse_map) = ({}, {}); > > my $device_change_handler = sub { > my $current = shift; > > + # happens during the clear + re-insertion of all interfaces after > + # the pinning changed, can be safely ignored > + return if $current->get_active() == -1; > + > my $new = $device_active_map->{ $current->get_active() }; > my $iface = $ipconf->{ifaces}->{$new}; > > @@ -389,6 +514,7 @@ sub create_ipconf_view { > return if defined($selected) && $iface->{name} eq $selected; > > Proxmox::Install::Config::set_mngmt_nic($iface->{name}); > + > $ipconf_entry_addr->set_text($iface->{inet}->{addr} || $iface->{inet6}->{addr}) > if $iface->{inet}->{addr} || $iface->{inet6}->{addr}; > $ipconf_entry_mask->set_text($iface->{inet}->{prefix} || $iface->{inet6}->{prefix}) > @@ -400,13 +526,6 @@ sub create_ipconf_view { > my $i = 0; > for my $index (sort keys $ipconf->{ifaces}->%*) { > my $iface = $ipconf->{ifaces}->{$index}; > - my $iter = $device_model->append(); > - my $symbol = "$iface->{state}" eq "UP" ? "\x{25CF}" : ' '; > - $device_model->set( > - $iter, > - 0 => $symbol, > - 1 => $get_device_desc->($iface), > - ); > $device_active_map->{$i} = $index; > $device_active_reverse_map->{ $iface->{name} } = $i; > > @@ -418,6 +537,9 @@ sub create_ipconf_view { > $i++; > } > > + # fill the combobox with entries > + $refresh_device_cb->(); > + > if (my $nic = Proxmox::Install::Config::get_mngmt_nic()) { > $initial_active_device_pos = $device_active_reverse_map->{$nic}; > } else { > @@ -443,7 +565,7 @@ sub create_ipconf_view { > $label->set_xalign(1.0); > > $grid->attach($label, 0, 0, 1, 1); > - $grid->attach($device_cb, 1, 0, 1, 1); > + $grid->attach($device_cb, 1, 0, 2, 1); > > my $fqdn = Proxmox::Install::Config::get_fqdn(); > my $hostname = $run_env->{network}->{hostname} || $iso_env->{product}; > @@ -452,17 +574,17 @@ sub create_ipconf_view { > > my ($host_label, $hostentry) = create_text_input($fqdn, 'Hostname (FQDN)'); > $grid->attach($host_label, 0, 1, 1, 1); > - $grid->attach($hostentry, 1, 1, 1, 1); > + $grid->attach($hostentry, 1, 1, 2, 1); > > $grid->attach($cidr_label, 0, 2, 1, 1); > - $grid->attach($cidr_box, 1, 2, 1, 1); > + $grid->attach($cidr_box, 1, 2, 2, 1); > > my $cfg_gateway = Proxmox::Install::Config::get_gateway(); > my $gateway = $cfg_gateway // $ipconf->{gateway} || '192.168.100.1'; > > my ($gw_label, $ipconf_entry_gw) = create_text_input($gateway, 'Gateway'); > $grid->attach($gw_label, 0, 3, 1, 1); > - $grid->attach($ipconf_entry_gw, 1, 3, 1, 1); > + $grid->attach($ipconf_entry_gw, 1, 3, 2, 1); > > my $cfg_dns = Proxmox::Install::Config::get_dns(); > my $dnsserver = $cfg_dns // $ipconf->{dnsserver} || $gateway; > @@ -470,7 +592,10 @@ sub create_ipconf_view { > my ($dns_label, $ipconf_entry_dns) = create_text_input($dnsserver, 'DNS Server'); > > $grid->attach($dns_label, 0, 4, 1, 1); > - $grid->attach($ipconf_entry_dns, 1, 4, 1, 1); > + $grid->attach($ipconf_entry_dns, 1, 4, 2, 1); > + > + $grid->attach($name_pin_checkbox, 1, 5, 1, 1); > + $grid->attach($name_pin_opts_button, 2, 5, 1, 1); > > $gtk_state->{inbox}->show_all; > set_next( > @@ -538,6 +663,8 @@ sub create_ipconf_view { > } > Proxmox::Install::Config::set_dns($dns_ip); > > + $gtk_state->{network_pinning_enabled} = !!$name_pin_checkbox->get_active(); > + > #print STDERR "TEST $ipaddress/$netmask $gateway_ip $dns_ip\n"; > > $step_number++; > @@ -573,6 +700,13 @@ sub create_ack_view { > > my $country = Proxmox::Install::Config::get_country(); > > + my $mngmt_nic = Proxmox::Install::Config::get_mngmt_nic(); > + my $iface = Proxmox::Install::RunEnv::get('network')->{interfaces}->{$mngmt_nic}; > + > + my $nic_mapping = Proxmox::Install::Config::get_network_interface_pin_map(); > + my $interface = > + $gtk_state->{network_pinning_enabled} ? $nic_mapping->{ $iface->{mac} } : $iface->{name}; > + > my %config_values = ( > __target_hd__ => join(' | ', $target_hds->@*), > __target_fs__ => Proxmox::Install::Config::get_filesys(), > @@ -580,7 +714,7 @@ sub create_ack_view { > __timezone__ => Proxmox::Install::Config::get_timezone(), > __keymap__ => Proxmox::Install::Config::get_keymap(), > __mailto__ => Proxmox::Install::Config::get_mailto(), > - __interface__ => Proxmox::Install::Config::get_mngmt_nic(), > + __interface__ => $interface, > __hostname__ => Proxmox::Install::Config::get_hostname(), > __cidr__ => Proxmox::Install::Config::get_cidr(), > __gateway__ => Proxmox::Install::Config::get_gateway(), > @@ -609,6 +743,12 @@ sub create_ack_view { > set_next( > undef, > sub { > + # before starting the install, unset the name pinning map if it > + # is disabled > + if (!$gtk_state->{network_pinning_enabled}) { > + Proxmox::Install::Config::set_network_interface_pin_map(undef); > + } > + > $step_number++; > create_extract_view(); > }, > @@ -1775,6 +1915,15 @@ if (!$initial_error && (scalar keys $run_env->{ipconf}->{ifaces}->%* == 0)) { > $initial_error = 1; > Proxmox::UI::display_html("nonics.htm"); > set_next("Reboot", sub { app_quit(0); }); > +} else { > + # we enable it by default for new installation > + $gtk_state->{network_pinning_enabled} = 1; > + > + # pre-fill the name mapping before starting > + my %mapping = map { > + $_->{mac} => DEFAULT_PIN_PREFIX . $_->{pinned_id} > + } values $run_env->{network}->{interfaces}->%*; > + Proxmox::Install::Config::set_network_interface_pin_map(\%mapping); > } > > create_intro_view() if !$initial_error; > diff --git a/test/Makefile b/test/Makefile > index 70a05be..bfe7674 100644 > --- a/test/Makefile > +++ b/test/Makefile > @@ -4,7 +4,8 @@ export PERLLIB=.. > > .PHONY: check > check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio \ > - test-zfs-get-pool-list test-parse-kernel-cmdline > + test-zfs-get-pool-list test-parse-kernel-cmdline \ > + test-validate-link-pin-map > > .PHONY: test-zfs-arc-max > test-zfs-arc-max: > @@ -29,3 +30,7 @@ test-zfs-get-pool-list: > .PHONY: test-parse-kernel-cmdline > test-parse-kernel-cmdline: > ./parse-kernel-cmdline.pl > + > +.PHONY: test-validate-link-pin-map > +test-validate-link-pin-map: > + ./validate-link-pin-map.pl > diff --git a/test/validate-link-pin-map.pl b/test/validate-link-pin-map.pl > new file mode 100755 > index 0000000..6386700 > --- /dev/null > +++ b/test/validate-link-pin-map.pl > @@ -0,0 +1,42 @@ > +#!/usr/bin/env perl > + > +use strict; > +use warnings; > + > +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"); > + > +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", > +); > + > +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/); > + > +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", > +); > + > +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", > +); > + > +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", > +); > + > +done_testing(); _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel