Gesendet: Montag, 20. Mai 2019 um 18:49 Uhr Von: "Tom Rymes" trymes@rymes.com An: Kein Empfänger Cc: development@lists.ipfire.org Betreff: Re: Aw: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click / Bug #10629
On 05/20/2019 12:42 PM, Bernhard Bitsch wrote:
Gesendet: Montag, 20. Mai 2019 um 18:03 Uhr Von: "Tom Rymes" trymes@rymes.com An: development@lists.ipfire.org Betreff: Re: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with 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 matter 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 click a second time.
That is already the case.
That's not true! Adding a new entry must retain the existing entries, what 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 change and complete the definition and adds this by clicking "add". A check for 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 static 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 shining 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.
We can guarantee the disjointness only, if we don't allow a definition which breaks this condition. My suggestion just should help to come out of this situation.
Actually, that's not technically accurate. If you read the bug I filed, you can see that writing the conf file in the proper way would also solve this issue.
Here is the bug I opened on that subject a number of years back: https://bugzilla.ipfire.org/show_bug.cgi?id=10629
Tom
Therefore I added the Bug # to the subject. Why can't we fixed this bugs together? They are located just in same code parts.
You can, but what with all of the complaints about messy, ugly, hard-to-maintain code, I figure that breaking things down into pieces would result in a faster, cleaner fix for the initial problem (#12050), which is a significant issue. The fix for #10629 can wait, as it is rarely an issue in practice, and that way we will be less likely to introduce new problems fixing 10629 when the true goal is to fix 12050.
Tom
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 "trial-and-error" patches, both. Thus a rigid rewrite of the "add2-ACTION" in dhcp.cgi could solve both. But if we correct bug #12050 that's enough for the moment. The solution is described, 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