Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click

[Michael Tremer <michael.tremer@ipfire.org>]

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

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.

> On 16 Apr 2019, at 17:41, Matthias Fischer <matthias.fischer(a)ipfire.org> wrote:
> 
> 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(+)
> 
> 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?

> 	    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.

> +	    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=31672dc8bdb223ebf425ff96be64318f2d68e0d7

> +	        &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