From mboxrd@z Thu Jan 1 00:00:00 1970
From: Leo Hofmann <hofmann@leo-andres.de>
To: development@lists.ipfire.org
Subject: Re: [PATCH] improve DHCP dynamic leases list usability
Date: Tue, 20 Oct 2020 23:55:25 +0200
Message-ID: <fb960cd7-dace-bdb4-7637-d1d8845eef88@leo-andres.de>
In-Reply-To:
<trinity-124ccef4-b3ee-4072-a8b7-26a4cc3f8dab-1603105343250@3c-app-gmx-bs66>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2074207192573519595=="
List-Id: <development.lists.ipfire.org>
--===============2074207192573519595==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Hi Michael, Hi Bernhard,
thank you for your feedback and support! As you suggested, I have split my pa=
tch and based it on the "next" branch.
I'll submit the two new patches in this thread soon. I hope it is okay to do =
it this way.
Best regards,
Leo
Am 19.10.2020 um 13:02 schrieb Bernhard Bitsch:
> Hi,
>
> nevertheless the functionality is ok. I just tested and reviewed by hand.
> I agree, that the separation of 'cosmetics' and 'extendings/error fixes' he=
lps a lot in review.
>
> Regards,
> Bernhard
>
>> Gesendet: Montag, 19. Oktober 2020 um 12:26 Uhr
>> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
>> An: "Leo Hofmann" <hofmann(a)leo-andres.de>
>> Cc: development(a)lists.ipfire.org
>> Betreff: Re: [PATCH] improve DHCP dynamic leases list usability
>>
>> 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 doe=
s 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=3Dflowed; 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 whic=
h 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 subm=
it 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-e=
xpired-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=3D'25%' align=3D'center'><a href=3D'$ENV{'SCRIPT_NAME'}?ETHER=
'><b>$tr{'mac address'}</b></a></th>
>>> <th width=3D'20%' align=3D'center'><a href=3D'$ENV{'SCRIPT_NAME'}?HOSTN=
AME'><b>$tr{'hostname'}</b></a></th>
>>> <th width=3D'25%' align=3D'center'><a href=3D'$ENV{'SCRIPT_NAME'}?ENDTI=
ME'><b>$tr{'lease expires'} (local time d/m/y)</b></a></th>
>>> -<th width=3D'5%' align=3D'center'><b>Add to fix leases<b></th>
>>> +<th width=3D'5%' align=3D'center'><b>Add to fix leases</b></th>
>>> </tr>
>>> END
>>> ;
>>>
>>> open(LEASES,"/var/state/dhcp/dhcpd.leases") or die "Can't open dhcp=
d.leases";
>>> - while ($line =3D <LEASES>) {
>>> - 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 <LEASES>) {
>>> + 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 "";
>>> + }
>>>
>>> - 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();
>>> + }
>>>
>>> - 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;
>>> + }
>>>
>>> - 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;
>>> + }
>>>
>>> - 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);
>>>
>>> my $id =3D 0;
>>> - my $col=3D"";
>>> + my $col =3D "";
>>> + my $separator_printed =3D 0;
>>> foreach my $key (sort leasesort keys %entries) {
>>> - print "<form method=3D'post' action=3D'/cgi-bin/dhcp.cgi'>\n";
>>> - my $hostname =3D &cleanhtml($entries{$key}->{HOSTNAME},"y");
>>> -
>>> - if ($id % 2) {
>>> - print "<tr>";
>>> - $col=3D"bgcolor=3D'$table1colour'";
>>> - }
>>> - else {
>>> - print "<tr>";
>>> - $col=3D"bgcolor=3D'$table2colour'";
>>> - }
>>> -
>>> - print <<END
>>> + 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 "<tr><td colspan=3D'5' bgcolor=3D'$table1colour'><=
hr size=3D'1'></td></tr>\n";
>>> + } else {
>>> + print "<tr><td colspan=3D'5' bgcolor=3D'$table2colour'><=
hr size=3D'1'></td></tr>\n";
>>> + }
>>> + $id++;
>>> + }
>>> +
>>> + print "<form method=3D'post' action=3D'/cgi-bin/dhcp.cgi'>\n";
>>> + if ($id % 2) {
>>> + print "<tr>";
>>> + $col=3D"bgcolor=3D'$table1colour'";
>>> + } else {
>>> + print "<tr>";
>>> + $col=3D"bgcolor=3D'$table2colour'";
>>> + }
>>> +
>>> + if($entries{$key}->{expired}) {
>>> + print <<END
>>> +<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_ADDR' value=
=3D'$entries{$key}->{IPADDR}' /><strike><i>$entries{$key}->{IPADDR}</i></stri=
ke></td>
>>> +<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_MAC' value=
=3D'$entries{$key}->{ETHER}' /><strike><i>$entries{$key}->{ETHER}</i></strike=
></td>
>>> +<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_REMARK' val=
ue=3D'$hostname' /><strike><i>$hostname_print<i><strike></td>
>>> +<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_ENABLED' va=
lue=3D'on' /><strike><i>$entries{$key}->{endtime_print}</i></strike></td>
>>> +END
>>> +;
>>> + } else {
>>> + print <<END
>>> <td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_ADDR' valu=
e=3D'$entries{$key}->{IPADDR}' />$entries{$key}->{IPADDR}</td>
>>> <td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_MAC' value=
=3D'$entries{$key}->{ETHER}' />$entries{$key}->{ETHER}</td>
>>> -<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_REMARK' val=
ue=3D'$hostname' />=C2=A0$hostname</td>
>>> -<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_ENABLED' va=
lue=3D'on' />
>>> +<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_REMARK' val=
ue=3D'$hostname' />$hostname_print</td>
>>> +<td align=3D'center' $col><input type=3D'hidden' name=3D'FIX_ENABLED' va=
lue=3D'on' />$entries{$key}->{endtime_print}</td>
>>> 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 "<strike>$enddate</strike>";
>>> - } else {
>>> - print "$enddate";
>>> - }
>>> - print <<END
>>> -</td><td $col><input type=3D'hidden' name=3D'ACTION' value=3D'$Lang::tr{=
'add'}2' /><input type=3D'submit' name=3D'SUBMIT' value=3D'$Lang::tr{'add'}' =
/>
>>> -</td></tr></form>
>>> +;
>>> + }
>>> +
>>> + print <<END
>>> +<td $col><input type=3D'hidden' name=3D'ACTION' value=3D'$Lang::tr{'add'=
}2' /><input type=3D'submit' name=3D'SUBMIT' value=3D'$Lang::tr{'add'}' /></t=
d>
>>> +</tr></form>
>>> END
>>> -;
>>> - $id++;
>>> - }
>>> +;
>>> + $id++;
>>> + }
>>>
>>> print "</table>";
>>> &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};
>>> - }
>>> + }
>>> }
>>> }
>>>
>>>
>>>
>>
--===============2074207192573519595==--