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

[Tom Rymes <trymes@rymes.com>]

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



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