* Re: Aw: Re: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
[not found] <trinity-22f9d088-08d9-4540-bb2b-37f63529dd0a-1555574876301@3c-app-gmx-bs26>
@ 2019-04-18 18:40 ` Tom Rymes
0 siblings, 0 replies; only message in thread
From: Tom Rymes @ 2019-04-18 18:40 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 12005 bytes --]
Bernhard,
While it would be possible to force the system to assign a new address
to a device when the user converts a dynamic lease to a static one, that
would be quite unusual, and contrary to what most users expect. Without
a doubt, it would be a pain, as the user would then have to point all
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
range excludes the statically defined addresses (see the bug description
for how this is done), or the user needs to be warned that the
configuration is unsupported and should be modified.
In the end, this is not an urgent matter, as it is a bit of a corner
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
> bug #10629. Our WUI just takes the MAC ( and IP ) of the dynamic lease
> and generates a fixed lease, which should be supplied with an IP from
> "fixed pool". Thus the next DHCP request from the client receives this
> fixed IP.
> The obsolete dynamic IP is reused for another MAC not before all other
> 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
> little dynamic range; it is possible to do the definition of fixed lease
> "a bit" after first access of the device.
> I didn't look at the lease process very deeply yet. Therefore I don't
> know whether it is possible to "undefine" the old <MAC,IP> pair in the
> dynamic pool. This would be the straight forward way to avoid these
> double definitions ( only one is active anyway! ).
> The file constituting the WUI page has "grown" over time and is somehow
> messy. Therefore the review will take some time for a adequate development.
> Bernhard
> *Gesendet:* Donnerstag, 18. April 2019 um 00:56 Uhr
> *Von:* "Tom Rymes" <trymes(a)rymes.com>
> *An:* "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de>
> *Cc:* development(a)lists.ipfire.org
> *Betreff:* Re: Aw: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed
> leases with one 'add' click
> The bug only describes the problem, I have not tried to fix it.
> Unfortunately, I lack the skills to implement the solution, and it is
> also a fairly minor problem. An easy workaround is to ensure that any
> static leases are not overlapping with the dynamic range. Also, this
> only manifests, IIRC, if the static device is not present on the network
> (away or powered off) when another device requests an address, and even
> then perhaps only when there are no other dynamic addresses available?
> Tom
>
> On Apr 17, 2019, at 6:15 PM, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de
> <mailto:Bernhard.Bitsch(a)gmx.de>> wrote:
>
> 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:* Donnerstag, 18. April 2019 um 00:01 Uhr
> *Von:* "Tom Rymes" <trymes(a)rymes.com <mailto:trymes(a)rymes.com>>
> *An:* "Bernhard Bitsch" <Bernhard.Bitsch(a)gmx.de
> <mailto:Bernhard.Bitsch(a)gmx.de>>
> *Cc:* BeBiMa <bbitsch(a)ipfire.org <mailto:bbitsch(a)ipfire.org>>,
> development(a)lists.ipfire.org <mailto:development(a)lists.ipfire.org>
> *Betreff:* Re: 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’t happening now (and is a bit convoluted). I did open a
> bug about this a while back, but it’s not a simple fix, I don’t think.
> https://bugzilla.ipfire.org/show_bug.cgi?id=10629
> Tom
>
> On Apr 17, 2019, at 5:49 PM, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de
> <mailto:Bernhard.Bitsch(a)gmx.de>> wrote:
>
> Hi,
> some explanations from the author:
>
> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr
>
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org
> <mailto:michael.tremer(a)ipfire.org>>
>
> An: "Matthias Fischer" <matthias.fischer(a)ipfire.org
> <mailto:matthias.fischer(a)ipfire.org>>
>
> Cc: development(a)lists.ipfire.org
> <mailto:development(a)lists.ipfire.org>, BeBiMa
> <bbitsch(a)ipfire.org <mailto: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
> <mailto:matthias.fischer(a)ipfire.org>> wrote:
>
> Signed-off-by: BeBiMa <bbitsch(a)ipfire.org
> <mailto:bbitsch(a)ipfire.org>>
>
> Reviewed-by: Matthias Fischer
> <matthias.fischer(a)ipfire.org
> <mailto: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
>
^ permalink raw reply [flat|nested] only message in thread