I, too, have found the operation of this page somewhat unintuitive. Also, while on the topic, I would like to point out that the current page will convert a dynamic lease to a static address without changing the address. This causes the static addresses to overlap with the dynamic pool, which is not supported and can result in duplicate addresses being assigned by DHCP. You can write the config file in such a way to exclude these addresses from the dynamic pool, but that isn’t happening now (and is a bit convoluted). I did open a bug about this a while back, but it’s not a simple fix, I don’t think.
https://bugzilla.ipfire.org/show_bug.cgi?id=10629
Tom
On Apr 17, 2019, at 5:49 PM, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:
Hi, some explanations from the author:
Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Matthias Fischer" matthias.fischer@ipfire.org Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
Hi,
Thanks Matthias for helping out here. However, I do not get the patch.
There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place.
The intention for the patch is to include new leases at the end with all fields defined by the admin. Up to now a new lease was added after an additional edit.
On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote:
Signed-off-by: BeBiMa bbitsch@ipfire.org Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org
html/cgi-bin/dhcp.cgi | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi index 675d80012..ba5b54f84 100644 --- a/html/cgi-bin/dhcp.cgi +++ b/html/cgi-bin/dhcp.cgi @@ -412,12 +412,16 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { }
my $key = 0;
- my $szc = scalar(@current2); CHECK:foreach my $line (@current2) { my @temp = split(/,/,$line); if($dhcpsettings{'KEY2'} ne $key) { # same MAC is OK on different subnets. This test is not complete because # if ip are not inside a known subnet, I don't warn. # Also it may be needed to put duplicate fixed lease in their right subnet definition..
if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) {last CHECK;}Why is this needed?
Check for existing lease. If <MAC,IP> is defined already we don't need to loop further.
foreach my $itf (@ITFs) { my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, $netsettings{"${itf}_NETADDRESS"},@@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); if ($dhcpsettings{'KEY2'} eq '') { #add or edit ?
This block here is not indented correctly.
I understand that the code is already very messy, but we should not make it worse either.
How should indention be done? By spaces, tabs? How many colums define an indent? The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. No problem. But this will produce a great number of patches. I made these modifications local, but generated the diff with "different spacing isn't a difference" option. This generated a short diff without real corrections only.
if($key == $szc) { #add@current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n";# sort newly added/modified entry&sortcurrent2;Are you sure we can sort here?
See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425...
Yes! Why not?
&General::log($Lang::tr{'fixed ip lease added'});$dhcpsettings{'KEY2'} = '';} else { #edit unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); &General::log($Lang::tr{'fixed ip lease added'}); # Enter edit mode $dhcpsettings{'KEY2'} = 0;}} else { @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; $dhcpsettings{'KEY2'} = ''; # End edit mode
-- 2.18.0
-Michael
Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). I am reviewing this piece of code at the moment and plan to suggest some modifications/corrections with formally legal patches. I hope this is ok for the core devs.
Regards, Bernhard