From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 14F9D1FF0C0 for ; Wed, 21 Oct 2026 17:36:48 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9CDE5214CC; Wed, 21 Oct 2026 17:36:46 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 21 Oct 2026 17:36:41 +0200 Message-Id: Subject: Re: [PATCH installer 10/15] gui: network: add bond setup options to advanced options dialog From: "Lukas Sichert" To: "Christoph Heiss" , References: <20261020112327.641633-1-c.heiss@proxmox.com> <20261020112327.641633-11-c.heiss@proxmox.com> In-Reply-To: <20261020112327.641633-11-c.heiss@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792597001748 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.610 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: JJL62S3MEKRVNQVY7CJZVEB3VOJLKFOB X-Message-ID-Hash: JJL62S3MEKRVNQVY7CJZVEB3VOJLKFOB X-MailFrom: l.sichert@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for sending this! The layout looks very good and is really intuitive, some nits inline: On 2026-10-20 13:23, Christoph Heiss wrote: > Signed-off-by: Christoph Heiss > proxinstall | 262 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 248 insertions(+), 14 deletions(-) > > diff --git a/proxinstall b/proxinstall > index e3a4b20..0b9f77b 100755 > --- a/proxinstall > +++ b/proxinstall > + my $on_mode_changed =3D sub { > + my $mode =3D $mode_combo->get_active_text(); > + $hash_policy_combo->set_sensitive($mode eq '802.3ad' || $mode eq= 'balance-xor'); > + $primary_if_input->set_sensitive($mode eq 'active-backup'); > + }; > + $mode_combo->signal_connect(changed =3D> $on_mode_changed); > + > + my $opts_grid =3D $create_label_widget_grid->([ > + ['Bond Mode', $mode_combo], > + ['Hash Policy', $hash_policy_combo], > + ['Primary Interface', $primary_if_input], > + ]); > + my $set_sensitive =3D sub { > + my ($active) =3D @_; > + > + $opts_grid->foreach(sub { > + $_[0]->set_sensitive($active); > + }); > + > + if ($active) { > + # if activating all inputs, observe the rules applied by the= mode change handler > + $on_mode_changed->(Proxmox::Install::Config::get_mngmt_bond_= opt('mode')); > + } Currently the font of the label of the bond primary textfield and the hash policy field are not greyed out if they are deactivated. This is especially confusing for the bond primary, because greyed out textfield and sensitive textfield look really similar. I think this could either be done in the on_mode_changed function or even in the foreach loop above. Also I am a bit confused as to why the on_mode changed function is called with a parameter, as it does not process one (see above). > + $if_label->set_sensitive($active); > + for my $cb (values %$checkboxes) { > + $cb->set_sensitive($active); > + } > + }; > + $set_sensitive->($gtk_state->{network_bonding_enabled}); > + > + $enable_checkbox->signal_connect( > + toggled =3D> sub { > + my $active =3D !!$enable_checkbox->get_active(); > + > + $gtk_state->{network_bonding_enabled} =3D $active; > + $set_sensitive->($active); > + }, > + ); > + > + my $refresh_if_names =3D sub { > + my ($pinning_active) =3D @_; > + my $mapping =3D Proxmox::Install::Config::get_network_interface_= pin_map(); > + > + for my $iface (values %$interfaces) { > + next if !defined($checkboxes->{ $iface->{mac} }); > + > + my $name =3D > + $pinning_active > + ? $name_mapping->{ $iface->{mac} } > + : $iface->{name}; Here probably $mapping was intended to be used instead of $name_mapping, but I am not sure if this will actually refresh interface names, as the map is only set on OK. > + my $label =3D "$name ($iface->{mac}, $iface->{driver}, $ifac= e->{state})"; > + $checkboxes->{ $iface->{mac} }->set_label($label); > + } > + }; > + $refresh_if_names->($gtk_state->{network_pinning_enabled}); > + > + my ($device_active_map, $device_active_reverse_map) =3D ({}, {}); > - my ($device_active_map, $device_active_reverse_map) =3D ({}, {}); I am not too sure about this refactor here. I think it should be done in a separate commit as it touches the non bonding setup as well. Also you use $device_active_map->{$i} =3D $label; compared to $device_active_map->{$i} =3D $name; before without changing the $device_change_handler. In the make check-pve preview, the nics get displayed twice once with virtio_net and once with unknown. > my $device_change_handler =3D sub { > my $current =3D shift; > =20 > @@ -660,8 +873,6 @@ sub create_ipconf_view { > my $i =3D 0; > for my $name (sort keys $network->{interfaces}->%*) { > my $iface =3D $network->{interfaces}->{$name}; > - $device_active_map->{$i} =3D $name; > - $device_active_reverse_map->{$name} =3D $i; > =20 > if (defined($default_gateway) && $name eq $default_gateway->{dev= }) { > # the chosen gateway is on this interface > @@ -683,11 +894,10 @@ sub create_ipconf_view { > $i++; > } > =20 > - # fill the combobox with entries > - $refresh_device_cb->(); > - > if (my $nic =3D Proxmox::Install::Config::get_mngmt_nic()) { > - $initial_active_device_pos =3D $device_active_reverse_map->{$nic= }; > + # $nic might be 'bond0' here, in which case this won't do much a= nyway > + $initial_active_device_pos =3D $device_active_reverse_map->{$nic= } > + if defined($device_active_reverse_map->{$nic}); > } else { > my $iface_name =3D $device_active_map->{$initial_active_device_p= os}; > Proxmox::Install::Config::set_mngmt_nic($iface_name); > @@ -711,6 +921,7 @@ sub create_ipconf_view { > =20 > $grid->attach($label, 0, 0, 1, 1); > $grid->attach($device_cb, 1, 0, 2, 1); > + $grid->attach($device_bond_label, 1, 0, 2, 1); > =20 > my $fqdn =3D Proxmox::Install::Config::get_fqdn(); > my $hostname =3D $run_env->{network}->{hostname} || $iso_env->{produ= ct}; > @@ -834,11 +1045,34 @@ sub create_ack_view { > my $country =3D Proxmox::Install::Config::get_country(); > =20 > my $mngmt_nic =3D Proxmox::Install::Config::get_mngmt_nic(); > - my $iface =3D Proxmox::Install::RunEnv::get('network')->{interfaces}= ->{$mngmt_nic}; > - > + my $all_ifaces =3D Proxmox::Install::RunEnv::get('network')->{interf= aces}; > + my $iface =3D $all_ifaces->{$mngmt_nic}; > my $nic_mapping =3D Proxmox::Install::Config::get_network_interface_= pin_map(); > - my $interface =3D > - $gtk_state->{network_pinning_enabled} ? $nic_mapping->{ $iface->= {mac} } : $iface->{name}; > + > + my $bond_opts =3D Proxmox::Install::Config::get_mngmt_bond_opt(); > + > + my $interface; > + if (Proxmox::Install::Config::get_mngmt_bond_enabled()) { > + my @member_names; > + > + # translate all bond members MACs to their final interface name,= either pinned or not > + for my $mac ($bond_opts->{members}->@*) { > + if ($gtk_state->{network_pinning_enabled}) { > + push @member_names, $nic_mapping->{$mac}; > + } else { > + my ($if) =3D grep { $_->{mac} eq $mac } values %$all_ifa= ces; > + push @member_names, $if->{name}; > + } > + } > + > + my $members =3D join(' | ', @member_names); > + $interface =3D "bond0 (mode: $bond_opts->{mode}, interfaces: $me= mbers)"; > + } else { > + $interface =3D > + $gtk_state->{network_pinning_enabled} > + ? $nic_mapping->{ $iface->{mac} } > + : $iface->{name}; > + } > =20 > my %config_values =3D ( > __target_hd__ =3D> join(' | ', $target_hds->@*), > --=20 > 2.55.0