Re: [PATCH] Add column with resolved hostname by IP address

Re: [PATCH] Add column with resolved hostname by IP address

[Michael Tremer]

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

Hello,

> On 28 Nov 2023, at 06:33, Sébastien GISLAIN <sebastien.gislain(a)gmail.com> wrote:
> 
> Hi,
> 
> I understand your reluctance for this development.
> 
> On the one hand, the resolution only concerns local hosts.

It is not guaranteed that any DNS lookups will only happen locally, but indeed rather likely.

> On the other hand, in my personal IPFire, with a Class C subnet on green network, I have 156 fixed leases and I don't see any slowdown.
> 
> Do you know of any IPFire deployments with class B or A subnets?

We have lots of users that have a couple of thousand clients behind the firewall, a couple of hundreds isn’t really that rare.

> Additionally, searching into the local hosts file requires more complex development...

Not the hosts file… The DHCP leases file. That is being parsed further down showing a long table. It contains hostnames that the clients have submitted and you could match them by their MAC/IP address. That way, you only parse one file and you get the hostname that the client thinks it has.

> Have a nice day,
> Sébastien
> 
> P.S.    Especially since I had prepared an update on the firewall log page (firewalllog.dat) which was even slower than that of the DHCP server page (dhcp.cgi)! 8'-(

What would that do?

> 
> Je comprends ta réticence pour cette évolution.
> D'une part, la résolution ne porte que sur des hôtes locaux.
> D'autre part, dans mon IPFire personnel, avec un sous-réseau de classe C, j'ai 156 baux fixes et je ne constate pas de ralentissement.
> Connais-tu des déploiements d'IPFire avec des sous-réseaux de classe B ou A ?
> 
> Le 24/11/2023 à 14:34, Michael Tremer a écrit :
>> Hello Sebastien,
>> 
>> Thank you very much for submitting this patch. However, I do have some concerns about this being merged.
>> 
>> If you only have a couple of leases, everything is fine, but there are people who have hundreds of DHCP clients on their network, and it has been reported before that this page is loading extremely slow. Adding a DNS request to this will probably make loading the page many times slower than it already is.
>> 
>> And to consider the performance hit, I do not quite know what the benefit is of showing this. Should we not rather parse the DHCP leases file and show the hostname that the client has submitted? That would show the same result, but it would be a lot faster.
>> 
>> -Michael
>> 
>> 
>>> On 9 Nov 2023, at 21:36, Sebastien GISLAIN <sebastien.gislain(a)free.fr> wrote:
>>> 
>>> ---
>>> html/cgi-bin/dhcp.cgi | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi
>>> index f45703f0b..aabf565d7 100755
>>> --- a/html/cgi-bin/dhcp.cgi
>>> +++ b/html/cgi-bin/dhcp.cgi
>>> @@ -21,6 +21,7 @@
>>> 
>>> use strict;
>>> use experimental 'smartmatch';
>>> +use IO::Socket;
>>> 
>>> # enable only the following on debugging purpose
>>> #use warnings;
>>> @@ -1009,6 +1010,7 @@ print <<END
>>> <tr>
>>> <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FETHER'><b>$Lang::tr{'mac address'}</b></a></th>
>>> <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FIPADDR'><b>$Lang::tr{'ip address'}</b></a></th>
>>> + <th width='20%' align='center'><b>$Lang::tr{'hostname'}</b></a></th>
>>> <th width='15%' align='center'><b>$Lang::tr{'remark'}</b></th>
>>> <th width='15%' class='boldbase' align='center'><b>next-server</b></th>
>>> <th width='15%' class='boldbase' align='center'><b>filename</b></th>
>>> @@ -1106,9 +1108,14 @@ foreach my $line (@current2) {
>>> $TAG4 = "class='red'" if ($dhcpsettings{'KEY2'} ne $key);
>>> }
>>> 
>>> + # resolved name (if exists)
>>> + my $iaddr = inet_aton($temp[1]);
>>> + my $rname = gethostbyaddr($iaddr, AF_INET);
>>> + if (!$rname || $rname eq "") { $rname = $Lang::tr{'lookup failed'}; }
>>> print <<END
>>> <td align='center' $col>$TAG2$temp[0]$TAG3</td>
>>> <td align='center' $TAG4 $col>$TAG0$temp[1]$TAG1</td>
>>> +<td align='center' $col>$rname&nbsp;</td>
>>> <td align='center' $col>$temp[6]&nbsp;</td>
>>> <td align='center' $col>$temp[3]&nbsp;</td>
>>> <td align='center' $col>$temp[4]&nbsp;</td>
>>> -- 
>>> 2.39.2
>>> 
>>> 
>> 
> 

[PATCH] Add column with resolved hostname by IP address

[Sebastien GISLAIN]

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

---
 html/cgi-bin/dhcp.cgi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi
index f45703f0b..aabf565d7 100755
--- a/html/cgi-bin/dhcp.cgi
+++ b/html/cgi-bin/dhcp.cgi
@@ -21,6 +21,7 @@
 
 use strict;
 use experimental 'smartmatch';
+use IO::Socket;
 
 # enable only the following on debugging purpose
 #use warnings;
@@ -1009,6 +1010,7 @@ print <<END
 <tr>
     <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FETHER'><b>$Lang::tr{'mac address'}</b></a></th>
     <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FIPADDR'><b>$Lang::tr{'ip address'}</b></a></th>
+	<th width='20%' align='center'><b>$Lang::tr{'hostname'}</b></a></th>
     <th width='15%' align='center'><b>$Lang::tr{'remark'}</b></th>
     <th width='15%' class='boldbase' align='center'><b>next-server</b></th>
     <th width='15%' class='boldbase' align='center'><b>filename</b></th>
@@ -1106,9 +1108,14 @@ foreach my $line (@current2) {
 	$TAG4 = "class='red'" if ($dhcpsettings{'KEY2'} ne $key);
     }
 
+	# resolved name (if exists)
+	my $iaddr = inet_aton($temp[1]);
+	my $rname = gethostbyaddr($iaddr, AF_INET);
+	if (!$rname || $rname eq "") { $rname = $Lang::tr{'lookup failed'}; }
     print <<END
 <td align='center' $col>$TAG2$temp[0]$TAG3</td>
 <td align='center' $TAG4 $col>$TAG0$temp[1]$TAG1</td>
+<td align='center' $col>$rname&nbsp;</td>
 <td align='center' $col>$temp[6]&nbsp;</td>
 <td align='center' $col>$temp[3]&nbsp;</td>
 <td align='center' $col>$temp[4]&nbsp;</td>
-- 
2.39.2

Re: [PATCH] Add column with resolved hostname by IP address

[Adolf Belka]

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

Hi Sebastien,

Thanks for the patch. I have tested this out and found a few quirks with it.

The table for that section is set at 100%. The original columns then added up to 100%.

Your patch added in an additional column at 20% making the total width now 120% but the overall table is still set at 100%. I think you need to adjust the width of each of the columns including the new column to come to a total of 100%.

In your line for the heading you have a closing <a/> tag for a hyperlink but there is no equivalent opening <a> tag in the line and no hyperlink defined. I am not sure if this is giving any problems but the closing <a/> tag is not needed there as there is no hyperlink.

When evaluating your patch I found that your Hostname column expands to keep the hostname on one line. This forces the other columns (next-server, filename and root path) to be squeezed up and made into multiple line entries. I think the hostname column should also be allowed to be a multi line entry to balance the columns up.

Eventually the other columns reach a min width and then the table expands outside of the overall frame of the IPFire page. This may be due to the columns totalling more than 100% but I am not familiar enough with html coding to be certain of that.

The line

if (!$rname || $rname eq "") { $rname = $Lang::tr{'lookup failed'}; }

has a Language lookup but the phrase 'lookup failed' has not been added into the English, German or French Language files.

Regards,

Adolf.

On 09/11/2023 22:36, Sebastien GISLAIN wrote:
> ---
>   html/cgi-bin/dhcp.cgi | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi
> index f45703f0b..aabf565d7 100755
> --- a/html/cgi-bin/dhcp.cgi
> +++ b/html/cgi-bin/dhcp.cgi
> @@ -21,6 +21,7 @@
>   
>   use strict;
>   use experimental 'smartmatch';
> +use IO::Socket;
>   
>   # enable only the following on debugging purpose
>   #use warnings;
> @@ -1009,6 +1010,7 @@ print <<END
>   <tr>
>       <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FETHER'><b>$Lang::tr{'mac address'}</b></a></th>
>       <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FIPADDR'><b>$Lang::tr{'ip address'}</b></a></th>
> +	<th width='20%' align='center'><b>$Lang::tr{'hostname'}</b></a></th>
>       <th width='15%' align='center'><b>$Lang::tr{'remark'}</b></th>
>       <th width='15%' class='boldbase' align='center'><b>next-server</b></th>
>       <th width='15%' class='boldbase' align='center'><b>filename</b></th>
> @@ -1106,9 +1108,14 @@ foreach my $line (@current2) {
>   	$TAG4 = "class='red'" if ($dhcpsettings{'KEY2'} ne $key);
>       }
>   
> +	# resolved name (if exists)
> +	my $iaddr = inet_aton($temp[1]);
> +	my $rname = gethostbyaddr($iaddr, AF_INET);
> +	if (!$rname || $rname eq "") { $rname = $Lang::tr{'lookup failed'}; }
>       print <<END
>   <td align='center' $col>$TAG2$temp[0]$TAG3</td>
>   <td align='center' $TAG4 $col>$TAG0$temp[1]$TAG1</td>
> +<td align='center' $col>$rname&nbsp;</td>
>   <td align='center' $col>$temp[6]&nbsp;</td>
>   <td align='center' $col>$temp[3]&nbsp;</td>
>   <td align='center' $col>$temp[4]&nbsp;</td>

Re: [PATCH] Add column with resolved hostname by IP address

[Michael Tremer]

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

Hello Sebastien,

Thank you very much for submitting this patch. However, I do have some concerns about this being merged.

If you only have a couple of leases, everything is fine, but there are people who have hundreds of DHCP clients on their network, and it has been reported before that this page is loading extremely slow. Adding a DNS request to this will probably make loading the page many times slower than it already is.

And to consider the performance hit, I do not quite know what the benefit is of showing this. Should we not rather parse the DHCP leases file and show the hostname that the client has submitted? That would show the same result, but it would be a lot faster.

-Michael

> On 9 Nov 2023, at 21:36, Sebastien GISLAIN <sebastien.gislain(a)free.fr> wrote:
> 
> ---
> html/cgi-bin/dhcp.cgi | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi
> index f45703f0b..aabf565d7 100755
> --- a/html/cgi-bin/dhcp.cgi
> +++ b/html/cgi-bin/dhcp.cgi
> @@ -21,6 +21,7 @@
> 
> use strict;
> use experimental 'smartmatch';
> +use IO::Socket;
> 
> # enable only the following on debugging purpose
> #use warnings;
> @@ -1009,6 +1010,7 @@ print <<END
> <tr>
>     <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FETHER'><b>$Lang::tr{'mac address'}</b></a></th>
>     <th width='20%' align='center'><a href='$ENV{'SCRIPT_NAME'}?FIPADDR'><b>$Lang::tr{'ip address'}</b></a></th>
> + <th width='20%' align='center'><b>$Lang::tr{'hostname'}</b></a></th>
>     <th width='15%' align='center'><b>$Lang::tr{'remark'}</b></th>
>     <th width='15%' class='boldbase' align='center'><b>next-server</b></th>
>     <th width='15%' class='boldbase' align='center'><b>filename</b></th>
> @@ -1106,9 +1108,14 @@ foreach my $line (@current2) {
> $TAG4 = "class='red'" if ($dhcpsettings{'KEY2'} ne $key);
>     }
> 
> + # resolved name (if exists)
> + my $iaddr = inet_aton($temp[1]);
> + my $rname = gethostbyaddr($iaddr, AF_INET);
> + if (!$rname || $rname eq "") { $rname = $Lang::tr{'lookup failed'}; }
>     print <<END
> <td align='center' $col>$TAG2$temp[0]$TAG3</td>
> <td align='center' $TAG4 $col>$TAG0$temp[1]$TAG1</td>
> +<td align='center' $col>$rname&nbsp;</td>
> <td align='center' $col>$temp[6]&nbsp;</td>
> <td align='center' $col>$temp[3]&nbsp;</td>
> <td align='center' $col>$temp[4]&nbsp;</td>
> -- 
> 2.39.2
>