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

[Bernhard Bitsch <Bernhard.Bitsch@gmx.de>]

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

Hi,
some explanations from the author:

> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Matthias Fischer" <matthias.fischer(a)ipfire.org>
> Cc: development(a)lists.ipfire.org, BeBiMa <bbitsch(a)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(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?

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

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