Re: [PATCH installer 10/15] gui: network: add bond setup options to advanced options dialog

["Lukas Sichert" <l.sichert@proxmox.com>]

Thanks for sending this! The layout looks very good and is really
intuitive, some nits inline:

On 2026-10-20 13:23, Christoph Heiss <c.heiss@proxmox.com> wrote:

> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>

>  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 = sub {
> +        my $mode = $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 => $on_mode_changed);
> +
> +    my $opts_grid = $create_label_widget_grid->([
> +        ['Bond Mode', $mode_combo],
> +        ['Hash Policy', $hash_policy_combo],
> +        ['Primary Interface', $primary_if_input],
> +    ]);

> +    my $set_sensitive = sub {
> +        my ($active) = @_;
> +
> +        $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 => sub {
> +            my $active = !!$enable_checkbox->get_active();
> +
> +            $gtk_state->{network_bonding_enabled} = $active;
> +            $set_sensitive->($active);
> +        },
> +    );
> +
> +    my $refresh_if_names = sub {
> +        my ($pinning_active) = @_;
> +        my $mapping = Proxmox::Install::Config::get_network_interface_pin_map();
> +
> +        for my $iface (values %$interfaces) {
> +            next if !defined($checkboxes->{ $iface->{mac} });
> +
> +            my $name =
> +                $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 = "$name ($iface->{mac}, $iface->{driver}, $iface->{state})";
> +            $checkboxes->{ $iface->{mac} }->set_label($label);
> +        }
> +    };
> +    $refresh_if_names->($gtk_state->{network_pinning_enabled});
> +

> +    my ($device_active_map, $device_active_reverse_map) = ({}, {});

> -    my ($device_active_map, $device_active_reverse_map) = ({}, {});

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} = $label;
compared to
$device_active_map->{$i} = $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 = sub {
>          my $current = shift;
>  
> @@ -660,8 +873,6 @@ sub create_ipconf_view {
>      my $i = 0;
>      for my $name (sort keys $network->{interfaces}->%*) {
>          my $iface = $network->{interfaces}->{$name};
> -        $device_active_map->{$i} = $name;
> -        $device_active_reverse_map->{$name} = $i;
>  
>          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++;
>      }
>  
> -    # 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};
> +        # $nic might be 'bond0' here, in which case this won't do much anyway
> +        $initial_active_device_pos = $device_active_reverse_map->{$nic}
> +            if defined($device_active_reverse_map->{$nic});
>      } else {
>          my $iface_name = $device_active_map->{$initial_active_device_pos};
>          Proxmox::Install::Config::set_mngmt_nic($iface_name);
> @@ -711,6 +921,7 @@ sub create_ipconf_view {
>  
>      $grid->attach($label, 0, 0, 1, 1);
>      $grid->attach($device_cb, 1, 0, 2, 1);
> +    $grid->attach($device_bond_label, 1, 0, 2, 1);
>  
>      my $fqdn = Proxmox::Install::Config::get_fqdn();
>      my $hostname = $run_env->{network}->{hostname} || $iso_env->{product};
> @@ -834,11 +1045,34 @@ 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 $all_ifaces = Proxmox::Install::RunEnv::get('network')->{interfaces};
> +    my $iface = $all_ifaces->{$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 $bond_opts = 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) = grep { $_->{mac} eq $mac } values %$all_ifaces;
> +                push @member_names, $if->{name};
> +            }
> +        }
> +
> +        my $members = join(' | ', @member_names);
> +        $interface = "bond0 (mode: $bond_opts->{mode}, interfaces: $members)";
> +    } else {
> +        $interface =
> +            $gtk_state->{network_pinning_enabled}
> +            ? $nic_mapping->{ $iface->{mac} }
> +            : $iface->{name};
> +    }
>  
>      my %config_values = (
>          __target_hd__ => join(' | ', $target_hds->@*),
> -- 
> 2.55.0