From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Bitsch To: development@lists.ipfire.org Subject: Aw: Re: [PATCH] improve DHCP dynamic leases list usability Date: Mon, 19 Oct 2020 13:02:23 +0200 Message-ID: In-Reply-To: <8DD762BC-3ED0-47E9-9C90-621B343D2B8F@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4813958072797186581==" List-Id: --===============4813958072797186581== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, nevertheless the functionality is ok. I just tested and reviewed by hand. I agree, that the separation of 'cosmetics' and 'extendings/error fixes' help= s a lot in review. Regards, Bernhard > Gesendet: Montag, 19. Oktober 2020 um 12:26 Uhr > Von: "Michael Tremer" > An: "Leo Hofmann" > Cc: development(a)lists.ipfire.org > Betreff: Re: [PATCH] improve DHCP dynamic leases list usability > > Good Morning Leo, >=20 > Thank you very much for submitting your patch. >=20 > 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. >=20 > 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=3Dflowed; space at the end of lines might b= e 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 >=20 > 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. >=20 > Could you please try to rebase this patch against the next branch and submi= t again? >=20 > Please let me know if you need any help with this. >=20 > Best, > -Michael >=20 > > On 18 Oct 2020, at 23:19, Leo Hofmann wrote: > >=20 > > 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. > >=20 > > See forum topic for context: https://community.ipfire.org/t/dhcp-server-e= xpired-leases/ > >=20 > > Signed-off-by: Leo-Andres Hofmann > >=20 > > --- > > config/cfgroot/header.pl | 167 +++++++++++++++++++++++---------------- > > 1 file changed, 97 insertions(+), 70 deletions(-) > >=20 > > 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 > > $tr{'mac address'} > > $tr{'hostname'} > > $tr{'lease expires'} (local time d/m/y) > > -Add to fix leases > > +Add to fix leases > > > > END > > ; > >=20 > > open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcpd= .leases"; > > - while ($line =3D ) { > > - next if( $line =3D~ /^\s*#/ ); > > - chomp($line); > > - @temp =3D split (' ', $line); > > - > > - if ($line =3D~ /^\s*lease/) { > > - $ip =3D $temp[1]; > > - #All field are not necessarily read. Clear everything > > - $endtime =3D 0; > > - $ether =3D ""; > > - $hostname =3D ""; > > - } > > + while (my $line =3D ) { > > + next if( $line =3D~ /^\s*#/ ); > > + chomp($line); > > + @temp =3D split (' ', $line); > > + > > + if ($line =3D~ /^\s*lease/) { > > + $ip =3D $temp[1]; > > + #All field are not necessarily read. Clear everything > > + $endtime =3D 0; > > + $endtime_print =3D ""; > > + $expired =3D 0; > > + $ether =3D ""; > > + $hostname =3D ""; > > + } > >=20 > > - if ($line =3D~ /^\s*ends/) { > > - $line =3D~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; > > - $endtime =3D timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); > > - } > > + if ($line =3D~ /^\s*ends/) { > > + $line =3D~ /(\d+)\/(\d+)\/(\d+) (\d+):(\d+):(\d+)/; > > + $endtime =3D timegm($6, $5, $4, $3, $2 - 1, $1 - 1900); > > + ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) = =3D localtime($endtime); > > + $endtime_print =3D sprintf ("%02d/%02d/%d %02d:%02d:%02d",$m= day,$mon+1,$year+1900,$hour,$min,$sec); > > + $expired =3D $endtime < time(); > > + } > >=20 > > - if ($line =3D~ /^\s*hardware ethernet/) { > > - $ether =3D $temp[2]; > > - $ether =3D~ s/;//g; > > - } > > + if ($line =3D~ /^\s*hardware ethernet/) { > > + $ether =3D $temp[2]; > > + $ether =3D~ s/;//g; > > + } > >=20 > > - if ($line =3D~ /^\s*client-hostname/) { > > - $hostname =3D "$temp[1] $temp[2] $temp[3]"; > > - $hostname =3D~ s/;//g; > > - $hostname =3D~ s/\"//g; > > - } > > + if ($line =3D~ /^\s*client-hostname/) { > > + $hostname =3D "$temp[1] $temp[2] $temp[3]"; > > + $hostname =3D~ s/;//g; > > + $hostname =3D~ s/\"//g; > > + } > >=20 > > - if ($line eq "}") { > > - @record =3D ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,'HOS= TNAME',$hostname); > > - $record =3D {}; # create a re= ference to empty hash > > - %{$record} =3D @record; # populate that h= ash with @record > > - $entries{$record->{'IPADDR'}} =3D $record; # add this to a= hash of hashes > > - } > > + if ($line eq "}") { > > + @record =3D ('IPADDR',$ip,'ENDTIME',$endtime,'ETHER',$ether,= 'HOSTNAME',$hostname,'endtime_print',$endtime_print,'expired',$expired); > > + $record =3D {}; # create a re= ference to empty hash > > + %{$record} =3D @record; # populate th= at hash with @record > > + $entries{$record->{'IPADDR'}} =3D $record; # add this to = a hash of hashes > > + } > > } > > close(LEASES); > >=20 > > my $id =3D 0; > > - my $col=3D""; > > + my $col =3D ""; > > + my $separator_printed =3D 0; > > foreach my $key (sort leasesort keys %entries) { > > - print "
\n"; > > - my $hostname =3D &cleanhtml($entries{$key}->{HOSTNAME},"y"); > > - > > - if ($id % 2) { > > - print ""; > > - $col=3D"bgcolor=3D'$table1colour'"; > > - } > > - else { > > - print ""; > > - $col=3D"bgcolor=3D'$table2colour'"; > > - } > > - > > - print < > + my $hostname =3D &cleanhtml($entries{$key}->{HOSTNAME},"y"); > > + my $hostname_print =3D $hostname; > > + if($hostname_print eq "") { #print blank space if no hostname is= found > > + $hostname_print =3D "=C2=A0=C2=A0=C2=A0"; > > + } > > + > > + # separate active and expired leases with a horizontal line > > + if(($entries{$key}->{expired}) && ($separator_printed =3D=3D 0))= { > > + $separator_printed =3D 1; > > + if ($id % 2) { > > + print "<= hr size=3D'1'>\n"; > > + } else { > > + print "<= hr size=3D'1'>\n"; > > + } > > + $id++; > > + } > > + > > + print "\n"; > > + if ($id % 2) { > > + print ""; > > + $col=3D"bgcolor=3D'$table1colour'"; > > + } else { > > + print ""; > > + $col=3D"bgcolor=3D'$table2colour'"; > > + } > > + > > + if($entries{$key}->{expired}) { > > + print < > +{IPADDR}' />$entries{$key}->{IPADDR} > > +{ETHER}' />$entries{$key}->{ETHER} > > +$hostname_print > > +$entries{$key}->{endtime_print} > > +END > > +; > > + } else { > > + print < > {IPADDR}' />$entries{$key}->{IPADDR} > > {ETHER}' />$entries{$key}->{ETHER} > > -=C2=A0$hostname > > - > > +$hostname_print > > +$entries{$key}->{endtime_print} > > END > > -; > > - > > - ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $dst) =3D loca= ltime ($entries{$key}->{ENDTIME}); > > - $enddate =3D sprintf ("%02d/%02d/%d %02d:%02d:%02d",$mday,$mon+1,$ye= ar+1900,$hour,$min,$sec); > > - > > - if ($entries{$key}->{ENDTIME} < time() ){ > > - print "$enddate"; > > - } else { > > - print "$enddate"; > > - } > > - print < > - > > - > > +; > > + } > > + > > + print < > + > > + > > END > > -; > > - $id++; > > - } > > +; > > + $id++; > > + } > >=20 > > print ""; > > &closebox(); > > @@ -499,11 +521,14 @@ sub leasesort { > > if ($qs eq 'IPADDR') { > > @a =3D split(/\./,$entries{$a}->{$qs}); > > @b =3D split(/\./,$entries{$b}->{$qs}); > > + $entries{$a}->{'expired'} <=3D> $entries{$b}->{'expired'} || > > ($b[0]<=3D>$a[0]) || > > ($b[1]<=3D>$a[1]) || > > ($b[2]<=3D>$a[2]) || > > ($b[3]<=3D>$a[3]); > > - }else { > > + ; > > + } else { > > + $entries{$a}->{'expired'} <=3D> $entries{$b}->{'expired'} || > > $entries{$b}->{$qs} cmp $entries{$a}->{$qs}; > > } > > } > > @@ -511,15 +536,17 @@ sub leasesort { > > { > > $qs=3D$dhcpsettings{'SORT_LEASELIST'}; > > if ($qs eq 'IPADDR') { > > - @a =3D split(/\./,$entries{$a}->{$qs}); > > + @a =3D split(/\./,$entries{$a}->{$qs}); > > @b =3D split(/\./,$entries{$b}->{$qs}); > > + $entries{$a}->{'expired'} <=3D> $entries{$b}->{'expired'} || > > ($a[0]<=3D>$b[0]) || > > - ($a[1]<=3D>$b[1]) || > > - ($a[2]<=3D>$b[2]) || > > + ($a[1]<=3D>$b[1]) || > > + ($a[2]<=3D>$b[2]) || > > ($a[3]<=3D>$b[3]); > > - }else { > > + } else { > > + $entries{$a}->{'expired'} <=3D> $entries{$b}->{'expired'} || > > $entries{$a}->{$qs} cmp $entries{$b}->{$qs}; > > - } > > + } > > } > > } > >=20 > >=20 > >=20 >=20 > --===============4813958072797186581==--