From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Bitsch To: development@lists.ipfire.org Subject: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click Date: Wed, 17 Apr 2019 23:49:00 +0200 Message-ID: In-Reply-To: <0ADEAEA1-EB9C-4A08-9FDE-23438ECFCEC6@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1486339736677670059==" List-Id: --===============1486339736677670059== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, some explanations from the author: > Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr > Von: "Michael Tremer" > An: "Matthias Fischer" > Cc: development(a)lists.ipfire.org, BeBiMa > 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 fiel= ds 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 wrote: > > > > Signed-off-by: BeBiMa > > Reviewed-by: Matthias Fischer > > --- > > 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 =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 be= cause > > # 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($dhcpsetti= ngs{'FIX_ADDR'}) eq lc($temp[1]))) { > > + last CHECK; > > + } > > Why is this needed? Check for existing lease. If is defined already we don't need to loo= p further. > > > foreach my $itf (@ITFs) { > > my $scoped =3D &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, > > $netsettings{"${itf}_NETADDRESS"}, > > @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2'= ) { > > $dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dhcpsettings{'FIX_= FILENAME'}); > > $dhcpsettings{'FIX_ROOTPATH'} =3D &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 inde= nt? The original file (and many ohters!) have an mixture of tab/space. Should we = patch that step by step ( tab=3D4 ) to increase readability. No problem. But this will produce a great number of patches. I made these mod= ifications local, but generated the diff with "different spacing isn't a diff= erence" option. This generated a short diff without real corrections only. > > + if($key =3D=3D $szc) { #add > > + @current2[$key] =3D "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FI= X_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsett= ings{'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=3D31672dc8= bdb223ebf425ff96be64318f2d68e0d7 > Yes! Why not? > > + &General::log($Lang::tr{'fixed ip lease added'}); > > + $dhcpsettings{'KEY2'} =3D ''; > > + } else { #edit > > unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADD= R'},$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'} =3D 0; > > + } > > } else { > > @current2[$dhcpsettings{'KEY2'}] =3D "$dhcpsettings{'FIX_MAC'},$dhcp= settings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR= '},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{= 'FIX_REMARK'}\n"; > > $dhcpsettings{'KEY2'} =3D ''; # 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 straigh= t 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 mod= ifications/corrections with formally legal patches. I hope this is ok for the= core devs. Regards, Bernhard --===============1486339736677670059==--