[pve-devel] [PATCH installer] show multiple disks in ack screen when zfs is used

[Thomas Lamprecht]

On 1/10/19 11:54 AM, Oguz Bektas wrote:
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>

result looks OK, some coding (style) nits inline:

> ---
>  proxinstall | 51 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/proxinstall b/proxinstall
> index ae4d1fe..1dfe84c 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -2187,6 +2187,26 @@ sub create_ipconf_view {
>      $hostentry->grab_focus();
>  }
>  
> +my $get_raid_devlist = sub {
> +
> +    my $dev_name_hash = {};
> +
> +    my $devlist = [];
> +    for (my $i = 0; $i < @$hds; $i++) {
> +	if (my $hd = $config_options->{"disksel$i"}) {
> +	    my ($disk, $devname, $size, $model) = @$hd;
> +	    die "device '$devname' is used more than once\n"
> +		if $dev_name_hash->{$devname};
> +	    $dev_name_hash->{$devname} = $hd;
> +	    push @$devlist, $hd;
> +	}
> +    }
> +
> +    return $devlist;
> +};
> +
> +
> +
>  sub create_ack_view {
>  
>      cleanup_view();
> @@ -2195,7 +2215,7 @@ sub create_ack_view {
>      my $ack_html = "${proxmox_libdir}/html/$steps[$step_number]->{html}";
>      my $html_data = file_get_contents($ack_template);
>  
> -    my %config_values = (
> +    my $config_values = {
>  	__target_hd__ => $target_hd,
>  	__target_fs__ => $config_options->{filesys},
>  	__country__ => $cmap->{country}->{$country}->{name},
> @@ -2208,9 +2228,16 @@ sub create_ack_view {
>  	__netmask__ => $netmask,
>  	__gateway__ => $gateway,
>  	__dnsserver__ => $dnsserver,
> -    );
> +    };

the above change from an hash to a hashref is not directly related to
the logical change of this patch and adds a bit noise.

>  
> -    while ( my ($k, $v) = each %config_values) {
> +    if ($config_options->{filesys} =~ m/zfs/) {

for completeness sake I'd do:
=~ /zfs|btrfs/

> +	my $raid_devlist = &$get_raid_devlist();

indent whitespace errors below

> +        my @raid_disks = map { $_->[1] } @{$raid_devlist};
> +        my $used_disks = join(' / ', @raid_disks);
> +        $config_values->{__target_hd__} = $used_disks;
> +    }
> +
> +    while ( my ($k, $v) = each %{$config_values}) {


hmm, maybe it would be easier to do something akin to:
(only quickly tested)

----8<----
diff --git a/proxinstall b/proxinstall
index ae4d1fe..8b41bfe 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2196,7 +2196,7 @@ sub create_ack_view {
     my $html_data = file_get_contents($ack_template);

     my %config_values = (
-       __target_hd__ => $target_hd,
+       __target_hd__ => join(' | ', @{$config_options->{target_hds}}),
        __target_fs__ => $config_options->{filesys},
        __country__ => $cmap->{country}->{$country}->{name},
        __timezone__ => $timezone,
@@ -3076,24 +3076,27 @@ sub create_hdsel_view {
     set_next(undef, sub {

        if ($config_options->{filesys} =~ m/zfs/) {
-           eval { get_zfs_raid_setup(); };
+           my ($devlist) = eval { get_zfs_raid_setup() };
            if (my $err = $@) {
                display_message("Warning: $err\n" .
                                "Please fix ZFS setup first.");
            } else {
+               $config_options->{target_hds} = [ map { $_->[1] } @$devlist ];
                $step_number++;
                create_country_view();
            }
        } elsif ($config_options->{filesys} =~ m/btrfs/) {
-           eval { get_btrfs_raid_setup(); };
+           my ($devlist) = eval { get_btrfs_raid_setup() };
            if (my $err = $@) {
                display_message("Warning: $err\n" .
                                "Please fix BTRFS setup first.");
            } else {
+               $config_options->{target_hds} = [ map { $_->[1] } @$devlist ];
                $step_number++;
                create_country_view();
            }
        } else {
+           $config_options->{target_hds} = [ $target_hd ];
            $step_number++;
            create_country_view();
        }
--

reuses get_*raid_setup calls already present and the ack screen does not need to
have file-system type depend logic (less coupling). What do you think?


>  	$html_data =~ s/$k/$v/g;
>      }
>  
> @@ -2917,24 +2944,6 @@ sub create_hdoption_view {
>      $dialog->destroy();
>  }
>  
> -my $get_raid_devlist = sub {
> -
> -    my $dev_name_hash = {};
> -
> -    my $devlist = [];
> -    for (my $i = 0; $i < @$hds; $i++) {
> -	if (my $hd = $config_options->{"disksel$i"}) {
> -	    my ($disk, $devname, $size, $model) = @$hd;
> -	    die "device '$devname' is used more than once\n"
> -		if $dev_name_hash->{$devname};
> -	    $dev_name_hash->{$devname} = $hd;
> -	    push @$devlist, $hd;
> -	}
> -    }
> -
> -    return $devlist;
> -};
> -
>  sub zfs_mirror_size_check {
>      my ($expected, $actual) = @_;
>  
>