Re: [PATCH] improve DHCP dynamic leases list usability

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 11164 bytes --]

Good Morning Leo,

Thank you very much for submitting your patch.

I agree that this is an issue that could be fixed. However, your patch does not apply. It seems to have been corrupted while being sent.

root(a)michael:/build/ipfire-2.x# pwclient git-am -s 3586
Applying patch #3586 using u'git am -s'
Description: improve DHCP dynamic leases list usability
warning: Patch sent with format=flowed; space at the end of lines might be lost.
Applying: improve DHCP dynamic leases list usability
error: corrupt patch at line 10
Patch failed at 0001 improve DHCP dynamic leases list usability
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
'git am' failed with exit status 128

You seem to have re-indented the code as well as making changes to it which makes reviewing very difficult. It would be better to do this in two steps: a) fix the code that is messy, and then b) extend it with new functionality.

Could you please try to rebase this patch against the next branch and submit again?

Please let me know if you need any help with this.

Best,
-Michael

> On 18 Oct 2020, at 23:19, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Some users have complained that displaying the active and expired leases in one
> unordered list is confusing. This patch improves the DHCP user interface by
> visually separating active and expired leases. The list is now sorted by active and
> expired leases and they are divided by a horizontal line.
> Sorting by IP/MAC/host/time and creating static leases remains unchanged.
> 
> See forum topic for context: https://community.ipfire.org/t/dhcp-server-expired-leases/
> 
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> 
> ---
>  config/cfgroot/header.pl | 167 +++++++++++++++++++++++----------------
>  1 file changed, 97 insertions(+), 70 deletions(-)
> 
> diff --git a/config/cfgroot/header.pl b/config/cfgroot/header.pl
> index 1046f5992..b5d153a55 100644
> --- a/config/cfgroot/header.pl
> +++ b/config/cfgroot/header.pl
> @@ -403,88 +403,110 @@ sub PrintActualLeases
>  <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ETHER'><b>$tr{'mac address'}</b></a></th>
>  <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?HOSTNAME'><b>$tr{'hostname'}</b></a></th>
>  <th width='25%' align='center'><a href='$ENV{'SCRIPT_NAME'}?ENDTIME'><b>$tr{'lease expires'} (local time d/m/y)</b></a></th>
> -<th width='5%' align='center'><b>Add to fix leases<b></th>
> +<th width='5%' align='center'><b>Add to fix leases</b></th>
>  </tr>
>  END
>  ;
> 
>      open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcpd.leases";
> -    while ($line = <LEASES>) {
> -    next if( $line =~ /^\s*#/ );
> -    chomp($line);
> -    @temp = split (' ', $line);
> -
> -    if ($line =~ /^\s*lease/) {
> -        $ip = $temp[1];
> -        #All field are not necessarily read. Clear everything
> -        $endtime = 0;
> -        $ether = "";
> -        $hostname = "";
> -    }
> +    while (my $line = <LEASES>) {
> +        next if( $line =~ /^\s*#/ );
> +        chomp($line);
> +        @temp = split (' ', $line);
> +
> +        if ($line =~ /^\s*lease/) {
> +            $ip = $temp[1];
> +            #All field are not necessarily read. Clear everything
> +            $endtime = 0;
> +            $endtime_print = "";
> +            $expired = 0;
> +            $ether = "";
> +            $hostname = "";
> +        }
> 
> -    if ($line =~ /^\s*ends/) {
> -        $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/;
> -        $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900);
> -    }
> +        if ($line =~ /^\s*ends/) {
> +            $line =~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/;
> +            $endtime = timegm($6, $5, $4, $3, $2 - 1, $1 - 1900);
> +            ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime($endtime);
> +            $endtime_print = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec);
> +            $expired = $endtime < time();
> +        }
> 
> -    if ($line =~ /^\s*hardware ethernet/) {
> -        $ether = $temp[2];
> -        $ether =~ s/;//g;
> -    }
> +        if ($line =~ /^\s*hardware ethernet/) {
> +            $ether = $temp[2];
> +            $ether =~ s/;//g;
> +        }
> 
> -    if ($line =~ /^\s*client-hostname/) {
> -        $hostname = "$temp[1] $temp[2] $temp[3]";
> -        $hostname =~ s/;//g;
> -        $hostname =~ s/\"//g;
> -    }
> +        if ($line =~ /^\s*client-hostname/) {
> +            $hostname = "$temp[1] $temp[2] $temp[3]";
> +            $hostname =~ s/;//g;
> +            $hostname =~ s/\"//g;
> +        }
> 
> -    if ($line eq "}") {
> -        @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname);
> -            $record = {};                                # create a reference to empty hash
> -        %{$record} = @record;                        # populate that hash with @record
> -        $entries{$record->{'IPADDR'}} = $record;       # add this to a hash of hashes
> -    }
> +        if ($line eq "}") {
> +            @record = ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOSTNAME',$hostname,'endtime_print',$endtime_print,'expired',$expired);
> +            $record = {};                                # create a reference to empty hash
> +            %{$record} = @record;                        # populate that hash with @record
> +            $entries{$record->{'IPADDR'}} = $record;    # add this to a hash of hashes
> +        }
>      }
>      close(LEASES);
> 
>      my $id = 0;
> -    my $col="";
> +    my $col = "";
> +    my $separator_printed = 0;
>      foreach my $key (sort leasesort keys %entries) {
> -    print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n";
> -    my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y");
> -
> -    if ($id % 2) {
> -        print "<tr>";
> -        $col="bgcolor='$table1colour'";
> -    }
> -    else {
> -        print "<tr>";
> -        $col="bgcolor='$table2colour'";
> -    }
> -
> -    print <<END
> +        my $hostname = &cleanhtml($entries{$key}->{HOSTNAME},"y");
> +        my $hostname_print = $hostname;
> +        if($hostname_print eq "") { #print blank space if no hostname is found
> +            $hostname_print = "&nbsp;&nbsp;&nbsp;";
> +        }
> +
> +        # separate active and expired leases with a horizontal line
> +        if(($entries{$key}->{expired}) && ($separator_printed == 0)) {
> +            $separator_printed = 1;
> +            if ($id % 2) {
> +                print "<tr><td colspan='5' bgcolor='$table1colour'><hr size='1'></td></tr>\n";
> +            } else {
> +                print "<tr><td colspan='5' bgcolor='$table2colour'><hr size='1'></td></tr>\n";
> +            }
> +            $id++;
> +        }
> +
> +        print "<form method='post' action='/cgi-bin/dhcp.cgi'>\n";
> +        if ($id % 2) {
> +            print "<tr>";
> +            $col="bgcolor='$table1colour'";
> +        } else {
> +            print "<tr>";
> +            $col="bgcolor='$table2colour'";
> +        }
> +
> +        if($entries{$key}->{expired}) {
> +            print <<END
> +<td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' /><strike><i>$entries{$key}->{IPADDR}</i></strike></td>
> +<td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' /><strike><i>$entries{$key}->{ETHER}</i></strike></td>
> +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' /><strike><i>$hostname_print<i><strike></td>
> +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' /><strike><i>$entries{$key}->{endtime_print}</i></strike></td>
> +END
> +;
> +        } else {
> +            print <<END
>  <td align='center' $col><input type='hidden' name='FIX_ADDR' value='$entries{$key}->{IPADDR}' />$entries{$key}->{IPADDR}</td>
>  <td align='center' $col><input type='hidden' name='FIX_MAC' value='$entries{$key}->{ETHER}' />$entries{$key}->{ETHER}</td>
> -<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' />&nbsp;$hostname</td>
> -<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' />
> +<td align='center' $col><input type='hidden' name='FIX_REMARK' value='$hostname' />$hostname_print</td>
> +<td align='center' $col><input type='hidden' name='FIX_ENABLED' value='on' />$entries{$key}->{endtime_print}</td>
>  END
> -;
> -
> -    ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = localtime ($entries{$key}->{ENDTIME});
> -    $enddate = sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$year+1900,$hour,$min,$sec);
> -
> -    if ($entries{$key}->{ENDTIME} < time() ){
> -        print "<strike>$enddate</strike>";
> -    } else {
> -        print "$enddate";
> -    }
> -    print <<END
> -</td><td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' />
> -</td></tr></form>
> +;
> +        }
> +
> +        print <<END
> +<td $col><input type='hidden' name='ACTION' value='$Lang::tr{'add'}2' /><input type='submit' name='SUBMIT' value='$Lang::tr{'add'}' /></td>
> +</tr></form>
>  END
> -;
> -    $id++;
> -    }
> +;
> +        $id++;
> +    }
> 
>      print "</table>";
>      &closebox();
> @@ -499,11 +521,14 @@ sub leasesort {
>          if ($qs eq 'IPADDR') {
>              @a = split(/\./,$entries{$a}->{$qs});
>              @b = split(/\./,$entries{$b}->{$qs});
> +            $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} ||
>              ($b[0]<=>$a[0]) ||
>              ($b[1]<=>$a[1]) ||
>              ($b[2]<=>$a[2]) ||
>              ($b[3]<=>$a[3]);
> -        }else {
> +            ;
> +        } else {
> +            $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} ||
>              $entries{$b}->{$qs} cmp $entries{$a}->{$qs};
>          }
>      }
> @@ -511,15 +536,17 @@ sub leasesort {
>      {
>          $qs=$dhcpsettings{'SORT_LEASELIST'};
>          if ($qs eq 'IPADDR') {
> -        @a = split(/\./,$entries{$a}->{$qs});
> +            @a = split(/\./,$entries{$a}->{$qs});
>              @b = split(/\./,$entries{$b}->{$qs});
> +            $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} ||
>              ($a[0]<=>$b[0]) ||
> -        ($a[1]<=>$b[1]) ||
> -        ($a[2]<=>$b[2]) ||
> +            ($a[1]<=>$b[1]) ||
> +            ($a[2]<=>$b[2]) ||
>              ($a[3]<=>$b[3]);
> -    }else {
> +        } else {
> +            $entries{$a}->{'expired'} <=> $entries{$b}->{'expired'} ||
>              $entries{$a}->{$qs} cmp $entries{$b}->{$qs};
> -    }
> +        }
>      }
>  }
> 
> 
>