Aw: Re: Re: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click / Bug #10629

[Bernhard Bitsch <Bernhard.Bitsch@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4557 bytes --]

> Gesendet: Montag, 20. Mai 2019 um 18:49 Uhr
> Von: "Tom Rymes" <trymes(a)rymes.com>
> An: Kein Empfänger
> Cc: development(a)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(a)rymes.com>
> >> An: development(a)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