From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Bitsch To: development@lists.ipfire.org Subject: Aw: Re: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click / Bug #10629 Date: Mon, 20 May 2019 21:13:07 +0200 Message-ID: In-Reply-To: <4742a2b1-ca05-3e30-c2d2-ad3eada462fa@rymes.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7465722641108958268==" List-Id: --===============7465722641108958268== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable > Gesendet: Montag, 20. Mai 2019 um 18:49 Uhr > Von: "Tom Rymes" > An: Kein Empf=C3=A4nger > Cc: development(a)lists.ipfire.org > Betreff: Re: Aw: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases wi= th one 'add' click / Bug #10629 > >=20 >=20 > On 05/20/2019 12:42 PM, Bernhard Bitsch wrote: > >=20 > >=20 > >> Gesendet: Montag, 20. Mai 2019 um 18:03 Uhr > >> Von: "Tom Rymes" > >> An: development(a)lists.ipfire.org > >> Betreff: Re: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases wit= h one 'add' click > >> > >> On 05/20/2019 10:21 AM, Bernhard Bitsch wrote: > >> > >> [snip] > >> > >>>>>> If you plan to change any behaviour of the CGI file, that is a matte= r open for discussion first and then work should start. > >>>>>> > >>>>> > >>>>> When is this discussed? I made a suggestion of changes of behaviour; = yet: > >>>>> - Adding a new fixed lease adds this directly, without having to clic= k a second time. > >>>> > >>>> That is already the case. > >>> > >>> That's not true! Adding a new entry must retain the existing entries, w= hat isn't the case ( see postings in forum ). > >> > >> I must chime in here that clicking the button add a dynamic lease > >> shouldn't require a second click, IMHO. Add the dynamic lease as fixed, > >> then present the user the option to change things, such as name, > >> address, etc. after it has already been added. > >> > >>>>> - Adding a dynamic lease to the fixed leases should work in two steps= : first the data from dynamic leases is copied to the edit fields, user can c= hange and complete the definition and adds this by clicking "add". A check fo= r disjunction of sets of fixed and dynamic leases would be possible. > >>>> > >>>> If you add it from the DHCP leases list, the static lease is meant to = be added right away, but you can still edit it to give it a better name or so= . This what currently seems to be broken. > >>>> > >>> > >>> Maybe this the intention of the actual implementation. The misfunction = lays in the bug, indeed. > >>> But my suggestion goes one step further. It isn't desirable to mix up s= tatic and dynamic leases. This is based on the mechanics, how dynamic leases = are found by dhcpd ( see man page ). IMHO, the problem with this is not shini= ng up immediately, but some times later ( with no modifications meantime ). A= two step process with check if the two sets are disjoint avoids this problem. > >> > >> [snip] > >> > >> While I would agree that keeping the dynamic and fixed leases physically > >> distinct is a good goal, that is a separate change from the newly > >> introduced bug, and should be handled separately. IPFire has allowed the > >> user to add fixed leases that overlap with the dynamic address scope for > >> as long as I have used the product, and it's not really a high-priority > >> issue IMHO, so mixing it up with the fix for the new bug seems like a > >> bad idea to me. > >> > >=20 > > We can guarantee the disjointness only, if we don't allow a definition wh= ich breaks this condition. > > My suggestion just should help to come out of this situation. >=20 > Actually, that's not technically accurate. If you read the bug I filed,=20 > you can see that writing the conf file in the proper way would also=20 > solve this issue. >=20 > >> Here is the bug I opened on that subject a number of years back: > >> https://bugzilla.ipfire.org/show_bug.cgi?id=3D10629 > >> > >> Tom > >> > >=20 > > Therefore I added the Bug # to the subject. > > Why can't we fixed this bugs together? They are located just in same code= parts. >=20 > You can, but what with all of the complaints about messy, ugly,=20 > hard-to-maintain code, I figure that breaking things down into pieces=20 > would result in a faster, cleaner fix for the initial problem (#12050),=20 > which is a significant issue. The fix for #10629 can wait, as it is=20 > rarely an issue in practice, and that way we will be less likely to=20 > introduce new problems fixing 10629 when the true goal is to fix 12050. >=20 > Tom >=20 Partly I agree with you, if are looking on symptoms only. If we look at the reason of these bugs, we can see that are introduced by "tr= ial-and-error" patches, both. Thus a rigid rewrite of the "add2-ACTION" in dh= cp.cgi could solve both. But if we correct bug #12050 that's enough for the moment. The solution is de= scribed, let's do it. About the messy code: I've reformated it by my own, for error hunting. Thus a= new version with better maintainibilty should be possible. Bernhard --===============7465722641108958268==--