From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rymes To: development@lists.ipfire.org Subject: Re: Aw: Re: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click Date: Thu, 18 Apr 2019 14:40:05 -0400 Message-ID: <26ceb0c3-9869-3a90-b9d7-e4a8ed7154e1@rymes.com> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7318610138820644410==" List-Id: --===============7318610138820644410== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Bernhard, While it would be possible to force the system to assign a new address=20 to a device when the user converts a dynamic lease to a static one, that=20 would be quite unusual, and contrary to what most users expect. Without=20 a doubt, it would be a pain, as the user would then have to point all=20 traffic to that device at a different IP address from that point forward. IMHO, either the file needs to be written in such a way that the dynamic=20 range excludes the statically defined addresses (see the bug description=20 for how this is done), or the user needs to be warned that the=20 configuration is unsupported and should be modified. In the end, this is not an urgent matter, as it is a bit of a corner=20 case. Let's take the time and get it right the first time. Tom On 04/18/2019 4:07 AM, Bernhard Bitsch wrote: > You are right. I tried to describe the problem shortly in a comment to=20 > bug #10629. Our WUI just takes the MAC ( and IP ) of the dynamic lease=20 > and generates a fixed lease, which should be supplied with an IP from=20 > "fixed pool". Thus the next=C2=A0 DHCP request from the client receives thi= s=20 > fixed IP. > The obsolete dynamic IP is reused for another MAC not before all other=20 > dynamic IPs are used. > My personal setup is as follows: ( I think this not too unusual ) > - all leases are fixed; allows clear rules using the IP of the device > - for easy addition of new devices / "guest access" on blue there is=20 > little dynamic range; it is possible to do the definition of fixed lease=20 > "a bit" after first access of the device. > I didn't look at the lease process very deeply yet. Therefore I don't=20 > know whether it is possible to "undefine" the old pair in the=20 > dynamic pool. This would be the straight forward way to avoid these=20 > double definitions ( only one is active anyway! ). > The file constituting the WUI page has "grown" over time and is somehow=20 > messy. Therefore the review will take some time for a adequate development. > Bernhard > *Gesendet:*=C2=A0Donnerstag, 18. April 2019 um 00:56 Uhr > *Von:*=C2=A0"Tom Rymes" > *An:*=C2=A0"Bernhard Bitsch" > *Cc:*=C2=A0development(a)lists.ipfire.org > *Betreff:*=C2=A0Re: Aw: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed=20 > leases with one 'add' click > The bug only describes the problem, I have not tried to fix it.=20 > Unfortunately, I lack the skills to implement the solution, and it is=20 > also a fairly minor problem. An easy workaround is to ensure that any=20 > static leases are not overlapping with the dynamic range. Also, this=20 > only manifests, IIRC, if the static device is not present on the network=20 > (away or powered off) when another device requests an address, and even=20 > then perhaps only when there are no other dynamic addresses available? > Tom >=20 > On Apr 17, 2019, at 6:15 PM, Bernhard Bitsch > wrote: >=20 > Thanks for mentioning this. > I saw this happening some days before, when I redefined some my > fixed leases lost during test of my first version of this fix. > I'll look at your bug. Did you use the original dhcpi.cgi or the > patched one? > Bernhard > *Gesendet:*=C2=A0Donnerstag, 18. April 2019 um 00:01 Uhr > *Von:*=C2=A0"Tom Rymes" > > *An:*=C2=A0"Bernhard Bitsch" > > *Cc:*=C2=A0BeBiMa >, > development(a)lists.ipfire.org > *Betreff:*=C2=A0Re: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed > leases with one 'add' click > 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=E2=80=99t happening now (and is a bit convoluted). I did o= pen a > bug about this a while back, but it=E2=80=99s not a simple fix, I don= =E2=80=99t think. > https://bugzilla.ipfire.org/show_bug.cgi?id=3D10629 > Tom >=20 > On Apr 17, 2019, at 5:49 PM, Bernhard Bitsch > wrote: >=20 > Hi, > some explanations from the author: >=20 > Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >=20 > Von: "Michael Tremer" > >=20 > An: "Matthias Fischer" > >=20 > Cc: development(a)lists.ipfire.org > , BeBiMa > > >=20 > Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases > with one 'add' click >=20 > Hi, >=20 > Thanks Matthias for helping out here. However, I do not get > the patch. >=20 > There is no explanation about what it is meant to do. The > intention already is that the lease is added in the first place. >=20 >=20 > 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. >=20 > On 16 Apr 2019, at 17:41, Matthias Fischer > > wrote: >=20 > Signed-off-by: BeBiMa > >=20 > Reviewed-by: Matthias Fischer > > >=20 > --- >=20 > html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >=20 > 1 file changed, 12 insertions(+) >=20 > diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi >=20 > index 675d80012..ba5b54f84 100644 >=20 > --- a/html/cgi-bin/dhcp.cgi >=20 > +++ b/html/cgi-bin/dhcp.cgi >=20 > @@ -412,12 +412,16 @@ if ($dhcpsettings{'ACTION'} eq > $Lang::tr{'add'}.'2') { >=20 > =C2=A0=C2=A0=C2=A0} >=20 > =C2=A0=C2=A0=C2=A0my $key =3D 0; >=20 > + =C2=A0=C2=A0=C2=A0my $szc =3D =C2=A0scalar(@current2); >=20 > =C2=A0=C2=A0=C2=A0CHECK:foreach my $line (@current2) { >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0my @temp =3D spl= it(/\,/,$line); >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if($dhcpsettings= {'KEY2'} ne $key) { >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0# same MAC is OK on differ= ent subnets. This test > is not complete because >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0# if ip are not inside a k= nown subnet, I don't warn. >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0# Also it may be needed to= put duplicate fixed > lease in their right subnet definition.. >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0if ((lc($dhcpsettings{'FI= X_MAC'}) eq > lc($temp[0])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq > lc($temp[1]))) { >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0l= ast CHECK; >=20 > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} >=20 > Why is this needed? >=20 >=20 > Check for existing lease. If is defined already we > don't need to loop further. >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0foreach my $itf (@ITFs) { >=20 > =C2=A0 =C2=A0 =C2=A0 =C2=A0my $scoped =3D > &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >=20 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$netsettings{"${itf}_NETADDRESS"}, >=20 > @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq > $Lang::tr{'add'}.'2') { >=20 > =C2=A0 =C2=A0$dhcpsettings{'FIX_FILENAME'} =3D > &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >=20 > =C2=A0 =C2=A0$dhcpsettings{'FIX_ROOTPATH'} =3D > &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >=20 > =C2=A0 =C2=A0if ($dhcpsettings{'KEY2'} eq '') { #add or ed= it ? >=20 > This block here is not indented correctly. >=20 > I understand that the code is already very messy, but we > should not make it worse either. >=20 > 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=3D4 ) 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. >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0if($key =3D=3D $szc) { #a= dd >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0@= current2[$key] =3D > "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpse= ttings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENA= ME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0#= sort newly added/modified entry >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0&= sortcurrent2; >=20 > Are you sure we can sort here? >=20 > See: > https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommitdiff;h=3D3= 1672dc8bdb223ebf425ff96be64318f2d68e0d7 >=20 >=20 > Yes! Why not? >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0&= General::log($Lang::tr{'fixed ip lease > added'}); >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0$= dhcpsettings{'KEY2'} =3D ''; >=20 > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else { #edit >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0unshift (@current2, > "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpse= ttings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENA= ME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0&General::log($Lang::tr{'f= ixed ip lease added'}); >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0# Enter edit mode >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0$dhcpsettings{'KEY2'} =3D = 0; >=20 > + =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0} >=20 > =C2=A0 =C2=A0} else { >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0@current2[$dhcpsettings{'K= EY2'}] =3D > "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpse= ttings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENA= ME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >=20 > =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0$dhcpsettings{'KEY2'} =3D = ''; =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0# End edit mode >=20 > -- >=20 > 2.18.0 >=20 > -Michael >=20 >=20 > 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 =C2=A0static > leases, for example, maybe this is invoked now by my modification). > I am =C2=A0reviewing 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. >=20 > Regards, > Bernhard >=20 --===============7318610138820644410==--