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

[Oguz Bektas]

----- 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 :)

> 
> 
> >  	$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

----- End forwarded message -----