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

[Thomas Lamprecht]

On 1/11/19 11:39 AM, Oguz Bektas wrote:
> ----- Forwarded message from Oguz Bektas <o.bektas at proxmox.com> -----
> 
> Date: Fri, 11 Jan 2019 11:36:32 +0100
> From: Oguz Bektas <o.bektas at proxmox.com>
> To: Thomas Lamprecht <t.lamprecht at proxmox.com>
> Subject: Re: [pve-devel] [PATCH installer] show multiple disks in ack screen when zfs is used
> User-Agent: NeoMutt/20170113 (1.7.2)
> 
> Hi.
> 
> Comments inline.
> 
> On Thu, Jan 10, 2019 at 06:09:04PM +0100, Thomas Lamprecht wrote:
>> 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?
> 
> I've tested your version a bit, seems to work as expected and I haven't noticed any
> issues with it (also cleaner). I'd be happy to provide a v2 of mine with
> the suggested changes or just apply yours :)

Much thanks for testing, I pushed my proposal with a Tested-By tag
of yours, as discussed off-list.

> 
>>
>>
>>>  	$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) = @_;
>>>  
>>>
>>
> 
> Cheers
>