From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject:
 Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
Date: Wed, 17 Apr 2019 10:31:03 +0100
Message-ID: <0ADEAEA1-EB9C-4A08-9FDE-23438ECFCEC6@ipfire.org>
In-Reply-To: <20190416164124.2290-1-matthias.fischer@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2472128893286772618=="
List-Id: <development.lists.ipfire.org>

--===============2472128893286772618==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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 i=
s that the lease is added in the first place.

> On 16 Apr 2019, at 17:41, Matthias Fischer <matthias.fischer(a)ipfire.org> =
wrote:
>=20
> Signed-off-by: BeBiMa <bbitsch(a)ipfire.org>
> Reviewed-by: Matthias Fischer <matthias.fischer(a)ipfire.org>
> ---
> html/cgi-bin/dhcp.cgi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>=20
> 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') {
>     }
> =09
>     my $key =3D 0;
> +    my $szc =3D  scalar(@current2);
>     CHECK:foreach my $line (@current2) {
>         my @temp =3D split(/\,/,$line);
>         if($dhcpsettings{'KEY2'} ne $key) {
>  	    # same MAC is OK on different subnets. This test is not complete beca=
use
> 	    # 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 su=
bnet definition..
> +	    if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcpsetting=
s{'FIX_ADDR'}) eq lc($temp[1]))) {
> +	        last CHECK;
> +       }

Why is this needed?

> 	    foreach my $itf (@ITFs) {
> 		my $scoped =3D &General::IpInSubnet($dhcpsettings{'FIX_ADDR'},
> 						  $netsettings{"${itf}_NETADDRESS"},=20
> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') {
> 	$dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dhcpsettings{'FIX_FI=
LENAME'});
> 	$dhcpsettings{'FIX_ROOTPATH'} =3D &Header::cleanhtml($dhcpsettings{'FIX_RO=
OTPATH'});
> 	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 w=
orse either.

> +	    if($key =3D=3D $szc) { #add
> +	        @current2[$key] =3D "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_=
ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettin=
gs{'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=3Dipfire-2.x.git;a=3Dcommitdiff;h=3D31672dc8bd=
b223ebf425ff96be64318f2d68e0d7

> +	        &General::log($Lang::tr{'fixed ip lease added'});
> +	        $dhcpsettings{'KEY2'} =3D '';
> +        } else { #edit
> 	    unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'=
},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'F=
IX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n");
> 	    &General::log($Lang::tr{'fixed ip lease added'});
>=20
> 	    # Enter edit mode
> 	    $dhcpsettings{'KEY2'} =3D 0;
> +	    }
> 	} else {
> 	    @current2[$dhcpsettings{'KEY2'}] =3D "$dhcpsettings{'FIX_MAC'},$dhcpse=
ttings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'}=
,$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'F=
IX_REMARK'}\n";
> 	    $dhcpsettings{'KEY2'} =3D '';       # End edit mode
> --=20
> 2.18.0
>=20

-Michael


--===============2472128893286772618==--