From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Bitsch To: development@lists.ipfire.org Subject: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click Date: Mon, 20 May 2019 22:27:54 +0200 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8881584208274850393==" List-Id: --===============8881584208274850393== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, to come to a short end, I've posted a solution in Bugzilla ( for #12050 only). Hope this helps. Bernhard > Gesendet: Montag, 20. Mai 2019 um 16:21 Uhr > Von: "Bernhard Bitsch" > An: "Michael Tremer" > Cc: "IPFire: Development-List" , florian.bu= ehrle(a)ipfire.org > Betreff: Aw: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one '= add' click > > Hi, >=20 > > Gesendet: Montag, 20. Mai 2019 um 11:37 Uhr > > Von: "Michael Tremer" > > An: "Bernhard Bitsch" > > Cc: "IPFire: Development-List" , florian.= buehrle(a)ipfire.org > > Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'ad= d' click > > > > Hi, > >=20 > > > On 18 May 2019, at 19:28, Bernhard Bitsch wr= ote: > > >=20 > > > Hi, > > >=20 > > >> Gesendet: Samstag, 18. Mai 2019 um 00:27 Uhr > > >> Von: "Michael Tremer" > > >> An: "Bernhard Bitsch" > > >> Cc: "IPFire: Development-List" , flori= an.buehrle(a)ipfire.org > > >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one = 'add' click > > >>=20 > > >> Hi, > > >>=20 > > >>> On 17 May 2019, at 23:21, Bernhard Bitsch = wrote: > > >>>=20 > > >>> Hi, > > >>>=20 > > >>> sorry if I were not exact enough. I am working with a matter of urgen= cy on the DHCP config problem and surely will contact Florian, if I've found = the error and a real fix for it. > > >>=20 > > >> No need to contact him directly. That is what this list is for. > > >>=20 > > >=20 > > > That is not he point. Maybe it is better to work directly together. > >=20 > > I doubt that. This is an Open Source project, we do things =E2=80=9Cin th= e open=E2=80=9D. > >=20 > > https://blog.ipfire.org/post/default-to-open-please > >=20 >=20 > Agreed! But I would prefere to discuss in the list mainly. Some things can = be cleared in IIRC quicker, okay. But then the solution should appear in the = list. This means, I do not have to check generally two communication channels= to be uptodate. >=20 > > >=20 > > >>> My post was just a heads up about this work. The rewriting will be th= e second step, after bug elimination. The purpose should be to faciliate futu= re bug fixes. > > >>=20 > > >> I do not think that it makes any sense to rewrite this CGI file. We ar= e going to get rid of the whole web UI in IPFire 3 and there are no new featu= res to expect in the DHCP area that it makes sense to tidy up to much. I am n= ot advocating to have messy code here, but it works, and it easily breaks if = you touch something. > > >>=20 > > >=20 > > > When will IPFire 3 come? > >=20 > > That is unknown. But every minute spend on IPFire 2 will delay it by anot= her minute. > >=20 > > > How should we find and correct errors in the meantime?=20 > > > About breaking by touching this messy code, I agree with you. But if th= e mess remains, the probability of importing new errors by "bug fixes" increa= ses. Just my opinion. > > >=20 > > >>> My idea for quick inclusion of fix: > > >>> Florian is working on it, therefore I'll discuss possible solutions w= ith him. He will commit these to the git repo, based on the actual dhcp.cgi f= ile. > > >>=20 > > >> Why don=E2=80=99t you say it here what you have found? > > >>=20 > > >=20 > > > Because I haven't found the error, yet. > > >=20 > > >>> When I am finished with the rewrite (including the error fix) I'll pu= blish it either here in the devel list or in git or both. > > >>> Is this ok? > > >>=20 > > >> That really depends on what you want to achieve here. If you are sendi= ng in patches that clean up code but do not change any behaviour you should c= ollect them in a git branch and send that branch as a patchset. Bug fixes sho= uld be sent separately if it makes sense. > > >>=20 > > >=20 > > > Okay, that is the way I thought of. At the moment Florian fixes the ac= tual error in the existing code. Independend of that I represent the rewrite = in git and/or devel list. > > >=20 > > >> If you plan to change any behaviour of the CGI file, that is a matter = open for discussion first and then work should start. > > >>=20 > > >=20 > > > When is this discussed? I made a suggestion of changes of behaviour; ye= t: > > > - Adding a new fixed lease adds this directly, without having to click = a second time. > >=20 > > That is already the case. >=20 > That's not true! Adding a new entry must retain the existing entries, what = isn't the case ( see postings in forum ). >=20 > >=20 > > > - 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 cha= nge and complete the definition and adds this by clicking "add". A check for = disjunction of sets of fixed and dynamic leases would be possible. > >=20 > > 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. T= his what currently seems to be broken. > >=20 >=20 > 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 stati= c 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 u= p immediately, but some times later ( with no modifications meantime ). A two= step process with check if the two sets are disjoint avoids this problem. >=20 > -Bernhard >=20 > > So you are not talking about changing any behaviour. This *is* the bug we= are talking about all along. > >=20 > > -Michael > >=20 > > >=20 > > > - Bernhard=20 > > >=20 > > >> Hope this makes sense. > > >>=20 > > >> -Michael > > >>=20 > > >>> Best, > > >>> Bernhard > > >>>=20 > > >>>> Gesendet: Freitag, 17. Mai 2019 um 21:18 Uhr > > >>>> Von: "Michael Tremer" > > >>>> An: "Bernhard Bitsch" > > >>>> Cc: "IPFire: Development-List" , flo= rian.buehrle(a)ipfire.org > > >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with on= e 'add' click > > >>>>=20 > > >>>> Hi, > > >>>>=20 > > >>>> This bug is currently assigned to Florian, because I have asked him = to have a look at it. > > >>>>=20 > > >>>> I do not really care who is working on this, but I would like everyo= ne to work together on it. > > >>>>=20 > > >>>> I would also like to stress that we have urgent fixes for loads of I= ntel processors in Core Update 132 and I think that this problem should also = be fixed in this update. So, please work on this with a matter of urgency. > > >>>>=20 > > >>>> Best, > > >>>> -Michael > > >>>>=20 > > >>>>> On 17 May 2019, at 11:58, Bernhard Bitsch wrote: > > >>>>>=20 > > >>>>> Hello, > > >>>>>=20 > > >>>>> just some news about this topic. > > >>>>>=20 > > >>>>> - Michael was right to refuse my quick and dirty patch. It did not = really solve the problem, maybe it introduced some new problems. Sorry for my= quick and dirty reaction. > > >>>>>=20 > > >>>>> - I wasn't satisfied with the situation being. The problem exists f= urthermore and isn't easy solved. Therefore I started a review and commenting= for my own. With some effort I think I've located the main error ( see my sh= ort post in bugzilla ). > > >>>>>=20 > > >>>>> Thus, you will read again from me about solutions for Bug #12050. > > >>>>> My current state is as follows: > > >>>>> I've added a bunch of comments for understanding the program. > > >>>>> The sort algorithm for fixed leases ( maybe dynamic leases also ) s= hould and can be optimised. > > >>>>> The operations "add new fixed lease", "edit existing fixed lease", = "add dynamic lease to fixed leases" must be verified and corrected. > > >>>>>=20 > > >>>>> Proposal for behaviour: > > >>>>> "add new fixed lease" : add a new entry with parameters to fixed le= ases list, sort according the existing order. It should not be necessary to e= dit it first. > > >>>>> "edit existing fixed lease": move parameters of selected entry to e= dit box. Highlight edited entry at his place ( if entry #12 should be edited,= row #12 is highlighted ). Change entry with new paramters, sort. > > >>>>> "add dynamic lease to fixed lease": move values of dynamic lease to= edit box. Editing is mandatory! We should not merge sets of fixed and dynami= c leases. Add new entry as new fixed lease with changed parameters, sort. > > >>>>>=20 > > >>>>> When I've succeeded in implementing these topics, I'll post the res= ult. > > >>>>> Because of the number of changes in source ( comments, enhancements= for readability, .... ) I would suggest a commit of approved file as a whole= , not just as single little patches. How can this be accomplished? > > >>>>> I think little patches here and there cannot solve the problem of l= ow maintainability of this file. > > >>>>>=20 > > >>>>> Regards, > > >>>>> Bernhard > > >>>>>=20 > > >>>>>> Gesendet: Samstag, 20. April 2019 um 18:35 Uhr > > >>>>>> Von: "Michael Tremer" > > >>>>>> An: "Bernhard Bitsch" > > >>>>>> Cc: "IPFire: Development-List" > > >>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with = one 'add' click > > >>>>>>=20 > > >>>>>> Hello, > > >>>>>>=20 > > >>>>>> I just wanted to share with the list that Bernhard has emailed me = in private, telling me that he no longer wants to pursue getting this patch m= erged. > > >>>>>>=20 > > >>>>>> I find this whole situation very frustrating, but of course I acce= pt his decision. > > >>>>>>=20 > > >>>>>> I guess we just have to agree that we disagree here. > > >>>>>>=20 > > >>>>>> Best, > > >>>>>> -Michael > > >>>>>>=20 > > >>>>>>> On 18 Apr 2019, at 21:37, Bernhard Bitsch wrote: > > >>>>>>>=20 > > >>>>>>>=20 > > >>>>>>>=20 > > >>>>>>>> Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr > > >>>>>>>> Von: "Michael Tremer" > > >>>>>>>> An: "Bernhard Bitsch" > > >>>>>>>> Cc: "IPFire: Development-List" > > >>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases wit= h one 'add' click > > >>>>>>>>=20 > > >>>>>>>> Hi, > > >>>>>>>>=20 > > >>>>>>>>> On 18 Apr 2019, at 13:54, Bernhard Bitsch wrote: > > >>>>>>>>>=20 > > >>>>>>>>> Hello, > > >>>>>>>>>=20 > > >>>>>>>>> I do not want to start any discussions about the way the projec= t does its work. > > >>>>>>>>> Therefore some (hopefully) short annotations below. > > >>>>>>>>>=20 > > >>>>>>>>>> Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr > > >>>>>>>>>> Von: "Michael Tremer" > > >>>>>>>>>> An: "Bernhard Bitsch" > > >>>>>>>>>> Cc: "IPFire: Development-List" > > >>>>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases w= ith one 'add' click > > >>>>>>>>>>=20 > > >>>>>>>>>> Hello, > > >>>>>>>>>>=20 > > >>>>>>>>>>> On 18 Apr 2019, at 12:23, Bernhard Bitsch wrote: > > >>>>>>>>>>>=20 > > >>>>>>>>>>> Hi, > > >>>>>>>>>>>=20 > > >>>>>>>>>>>> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr > > >>>>>>>>>>>> Von: "Michael Tremer" > > >>>>>>>>>>>> An: "Bernhard Bitsch" > > >>>>>>>>>>>> Cc: BeBiMa , "IPFire: Development-List= " > > >>>>>>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases= with one 'add' click > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> Hi, > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>>> On 17 Apr 2019, at 22:49, Bernhard Bitsch wrote: > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> Hi, > > >>>>>>>>>>>>> some explanations from the author: > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr > > >>>>>>>>>>>>>> Von: "Michael Tremer" > > >>>>>>>>>>>>>> An: "Matthias Fischer" > > >>>>>>>>>>>>>> Cc: development(a)lists.ipfire.org, BeBiMa > > >>>>>>>>>>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leas= es with one 'add' click > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> Hi, > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> Thanks Matthias for helping out here. However, I do not ge= t the patch. > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> There is no explanation about what it is meant to do. The = intention already is that the lease is added in the first place. > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> 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. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> Those comments *must* be in the code. Nobody goes through th= ousands of emails on a mailing list to find out what is actually intended in = the code. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> You're right. But knowing, there should be some more work on = this page, this wasn't my main effort for this quick and short solution. For = this special case I regarded the forum post and the bugzilla entry to be suff= icient documentation, for the moment. > > >>>>>>>>>>=20 > > >>>>>>>>>> No. > > >>>>>>>>>>=20 > > >>>>>>>>>> Every commit must contain itself. > > >>>>>>>>>>=20 > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> On 16 Apr 2019, at 17:41, Matthias Fischer wrote: > > >>>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> Signed-off-by: BeBiMa > > >>>>>>>>>>>>>>> Reviewed-by: Matthias Fischer > > >>>>>>>>>>>>>>> --- > > >>>>>>>>>>>>>>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ > > >>>>>>>>>>>>>>> 1 file changed, 12 insertions(+) > > >>>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> 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 $Lan= g::tr{'add'}.'2') { > > >>>>>>>>>>>>>>> } > > >>>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> my $key =3D 0; > > >>>>>>>>>>>>>>> + my $szc =3D scalar(@current2); > > >>>>>>>>>>>>>>> CHECK:foreach my $line (@current2) { > > >>>>>>>>>>>>>>> my @temp =3D 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; > > >>>>>>>>>>>>>>> + } > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> Why is this needed? > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> Check for existing lease. If is defined already we= don't need to loop further. > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> foreach my $itf (@ITFs) { > > >>>>>>>>>>>>>>> my $scoped =3D &General::IpInSubnet($dhcpsettings{'FIX_= ADDR'}, > > >>>>>>>>>>>>>>> $netsettings{"${itf}_NETADDRESS"}, > > >>>>>>>>>>>>>>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lan= g::tr{'add'}.'2') { > > >>>>>>>>>>>>>>> $dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dh= cpsettings{'FIX_FILENAME'}); > > >>>>>>>>>>>>>>> $dhcpsettings{'FIX_ROOTPATH'} =3D &Header::cleanhtml($dh= cpsettings{'FIX_ROOTPATH'}); > > >>>>>>>>>>>>>>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> This block here is not indented correctly. > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> I understand that the code is already very messy, but we s= hould not make it worse either. > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> How should indention be done? By spaces, tabs? How many col= ums define an indent? > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> In the patch it looks like you wrapped the block into anothe= r if condition. So it has to be indented to make that obvious. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> Maybe this generated by different editors, I used. Indention = of the patch isn't worse than the existing indentation. Not being a final ver= sion ( see above ) it is sufficient, in my opinion. > > >>>>>>>>>>=20 > > >>>>>>>>>> Do *not* submit non-final versions. There is no point in it. A= patch is meant to go into a final release. We do not want beta stuff in ther= e. > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Okay. Got it. I should have sent my modified dhcp.cgi because o= f lack of a working git repo on my site. > > >>>>>>>>=20 > > >>>>>>>> Please learn how to use git and use =E2=80=9Cgit send-email=E2= =80=9D. There is an introduction on the wiki on how to set it up and there is= tons of resources on the Internet that explain Git to you in the form of how= tos and videos. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> Sorry, I know about git. It is not the lack of knowledge, but my = limited equipment. You invite to donate for the project, which is very urgent= , I think. On the other hand I've tried to help with limited sources and with= out monetary assistance, till now. If this isn't possible, it is okay for me.= I'll retire to the status "consuming user". > > >>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>>>> The original file (and many ohters!) have an mixture of tab= /space. Should we patch that step by step ( tab=3D4 ) to increase readability. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> This is not about tabs or spaces. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> Sure! This mixture doesnt't matter only in case of "tab=3D4" = definition, which I didn't find in the docs (yet). > > >>>>>>>>>>> I'll use this definition for further development ( and format= ed dhcpi.cgi in my work copy in the neighbourhood of the change, yet ). > > >>>>>>>>>>> This means that further patches may contain such "cosmetics",= if they are necessary to understand the code working on. > > >>>>>>>>>>=20 > > >>>>>>>>>> If you insist to take this conversation down this route, then = feel free to do so. We do not have a policy that commands spaces. > > >>>>>>>>>>=20 > > >>>>>>>>>> I liked you the coding style.=20 > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Especially in this file I found a messy mixture of tabs and spa= ces. Therefore my statement. If we commit us to tab=3D4 these spaces can be e= liminated. > > >>>>>>>>=20 > > >>>>>>>> We inherited some code from IPCop that we did not clean up. > > >>>>>>>>=20 > > >>>>>>>> So the guideline is to use the coding style used in the particul= ar file. Do not re-indent the whole file. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> If that's the opinion of the majority of the developers, live wit= h it. One reason of the ending of IPCop development was this rigid interpreta= tion of "don't touch a running system", IMHO. > > >>>>>>>=20 > > >>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>>>> No problem. But this will produce a great number of patches= . I made these modifications local, but generated the diff with "different sp= acing isn't a difference" option. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> Please just use =E2=80=9Cgit diff=E2=80=9D. It has a variety= of settings that are just right. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> I'll use this in future. Because I didn't do the modification= in a git repo, but in the working system, I didn't realize the possibility o= f diffing two arbitrary files with "git diff". Sorry. > > >>>>>>>>>>=20 > > >>>>>>>>>> Of course you would have the files in your working system. How= else would you test? > > >>>>>>>>>>=20 > > >>>>>>>>>> But for development purposes git is being used. It is the stan= dard. Otherwise patches won=E2=80=99t apply. There is no point in sending pat= ches that other developers cannot apply. Use Git. > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Ok. If I don't have a git repo myself, I send the modified sour= ce. Each developer with an actual git repo can apply it by commit. Right? > > >>>>>>>>=20 > > >>>>>>>> Please learn how to use git. > > >>>>>>>>=20 > > >>>>>>>> Of course you could send modified files around, but that does no= t work. Of course you could send instructions to other people on how to chang= e files, but that doesn=E2=80=99t work either. > > >>>>>>>>=20 > > >>>>>>>> I personally won=E2=80=99t do any work of trying to apply any pa= tches that are send in other forms. There is also reasons that we send patche= s inline: We can comment on them. > > >>>>>>>>=20 > > >>>>>>>> Please learn how to use git. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> All said about this above. > > >>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>>>> This generated a short diff without real corrections only. > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> + if($key =3D=3D $szc) { #add > > >>>>>>>>>>>>>>> + @current2[$key] =3D "$dhcpsettings{'FIX_MAC'},$= dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXT= ADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsetti= ngs{'FIX_REMARK'}\n"; > > >>>>>>>>>>>>>>> + # sort newly added/modified entry > > >>>>>>>>>>>>>>> + &sortcurrent2; > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> Are you sure we can sort here? > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> See: https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommit= diff;h=3D31672dc8bdb223ebf425ff96be64318f2d68e0d7 > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> Yes! Why not? > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> Because of the commit I referred to. > > >>>>>>>>>>>=20 > > >>>>>>>>>>> Didn't understand this commit, because of lack of commentatio= n. ;) > > >>>>>>>>>>=20 > > >>>>>>>>>> And you didn=E2=80=99t ask any questions then? > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Ok. I ask you now. Why doesn't it function? > > >>>>>>>>=20 > > >>>>>>>> Well, as the patch there states the key is saved and used later.= When the leases file is sorted, the key changes but is not updated. That lea= d to the case that you edited a different lease than you wanted. > > >>>>>>>>=20 > > >>>>>>>> I did not test this, but I could imagine that this could happen = here, too. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> I know from reading and understandig the code, that this works. T= he approval by Matthias Fischer shows, that I'm right. See the related forum = thread. > > >>>>>>> Software development and code review is no field of imagination. > > >>>>>>>=20 > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> + &General::log($Lang::tr{'fixed ip lease added'}= ); > > >>>>>>>>>>>>>>> + $dhcpsettings{'KEY2'} =3D ''; > > >>>>>>>>>>>>>>> + } else { #edit > > >>>>>>>>>>>>>>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcps= ettings{'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'}); > > >>>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>> # Enter edit mode > > >>>>>>>>>>>>>>> $dhcpsettings{'KEY2'} =3D 0; > > >>>>>>>>>>>>>>> + } > > >>>>>>>>>>>>>>> } else { > > >>>>>>>>>>>>>>> @current2[$dhcpsettings{'KEY2'}] =3D "$dhcpsettings{= 'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettin= gs{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'= },$dhcpsettings{'FIX_REMARK'}\n"; > > >>>>>>>>>>>>>>> $dhcpsettings{'KEY2'} =3D ''; # End edit mode > > >>>>>>>>>>>>>>> -- > > >>>>>>>>>>>>>>> 2.18.0 > > >>>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>> -Michael > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>>=20 > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> Again, my intention was a quick resolution for the behaviou= r mentioned in the forum post, which initiated my code review of dhcpi.cgi > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> Okay, I will try to explain this one last time - not just on= ly for you, but generally: > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> I am not interested in quick and dirty solutions. That is ho= w you break things. I am interested in well-documented, peer-reviewed and tes= ted code. We are creating some piece of high-quality software - or aim to do = so at least - and there is no space for quick and dirty. > > >>>>>>>>>>>=20 > > >>>>>>>>>>> This special modication wasn't intended as quick and dirty, b= ut as quick and stable and intuitive. Sorry, the first version was really a '= dirty shot'. But I wanted to present this as soon as possible for review and = test, which was done by Matthias Fischer.=20 > > >>>>>>>>>>> I agree fully with your aim, but this means readability also!= This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this ac= cepted? > > >>>>>>>>>>=20 > > >>>>>>>>>> You cannot submit a patch that does not fulfil the guidelines = and then promise to fix it later. There are obvious problems with the patch a= nd there are open questions. Why would we merge something that clearly adds m= ore problems than it solves? > > >>>>>>>>>>=20 > > >>>>>>>>>> So no, it is not accepted. > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Where are the problems? > > >>>>>>>>> Be more specific please. > > >>>>>>>>=20 > > >>>>>>>> Read my first email on the patch. That is as specific as it gets. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> I'll do this. But I don't know whether this changes the my opinio= n about the fixed problem. > > >>>>>>>=20 > > >>>>>>>>>>>> Please figure out how to set up a local Git repository, how = to set up a branch, how to commit things and what rules there are to follow. = Then find out on how to submit a patchset to the mailing list - after it has = been tested. It is all in the link that I sent you. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> This is not a competition about who can submit patches the f= astest. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> As stated before, the fact I didn't use a git repo has reason= s in my personal installation at the moment. Therefore your reminder about th= at isn't relevant for this single case. I also stated, that I'll surely use g= it for greater modifications I do on this topic ( and others ). > > >>>>>>>>>>> I didn't want to start a competition. It was just one more po= st in the forum about not intuitive behaviour of the DHCP WUI page, resulting= in a bugzilla topic by Matthias.=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>>>> I found a couple of behaviours of this page, which are not = obvious or straight forward ( adding dynamic leases to static leases, for ex= ample, maybe this is invoked now by my modification). > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> What are those? Why are we not talking about those first and= then come up with fixes? The whole DHCP page is a mess. I am not sure if it = can even be fixed or of things won=E2=80=99t just become worse. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> Being an experienced software developer, I think this is poss= ible and I am just working on this. I'll discuss these topics in the list bef= ore sending patches, being aware this is a community project this many opinio= ns. > > >>>>>>>>>>=20 > > >>>>>>>>>> This is not necessarily about opinions. It is about what we ca= n support in the end and where we all want to invest our time. > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Helping users with known problems, which could be resolved, is = wating time also. In the moment I do this mainly. > > >>>>>>>>>=20 > > >>>>>>>>>> Think about that we are all in the same boat and we want to im= prove IPFire wherever we can. But we need to talk about things because one se= t of eyes often is not enough. Something that works for one person does not w= ork for another. We can never break backwards-compatibility. > > >>>>>>>>>>=20 > > >>>>>>>>>> I personally want to see this static lease bug fixed. I am try= ing to help you to develop a good solution that we do not have to worry about= in the future. I am also enforcing the rules that we all have come up with s= ome long time ago and that work for this project. That way we will hopefully = all benefit from this. > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> Why didn't you just say this. A request for the whole file woul= d have been enough. BTW the file can be found in the forum post. > > >>>>>>>>=20 > > >>>>>>>> There are problems *in* the file. I raised questions. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> see above. > > >>>>>>>=20 > > >>>>>>>>>> But I do not want to have endless discussions on this list abo= ut why the rules are as they are. If there is a constructive proposal to make= things easier for all then we are all of course open for this. On the other = hand debating what is the standard now is just a waste of time. I am happy to= explain this, but I am not willing to debate them or compromise on this. Aga= in, this is a general statement and not limited to this conversation. > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> You started this discussion ( once more ). > > >>>>>>>>=20 > > >>>>>>>> Okay, I tried to be helpful here. > > >>>>>>>>=20 > > >>>>>>>> If you prefer to insist that this patch is not being amended and= if you also prefer to point fingers, keep doing it. > > >>>>>>>>=20 > > >>>>>>>> I will remember this and keep my comments to myself in the futur= e then. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> There is no need to behave like this. I'll be quiet myself upon s= ource quality and proposed fixes for bugs. > > >>>>>>>=20 > > >>>>>>>>>>>>> I am reviewing this piece of code at the moment and plan t= o suggest some modifications/corrections with formally legal patches. I hope = this is ok for the core devs. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>> As I said, please follow the process. It is there for a reas= on. This is not to make things difficult. In fact it makes things a lot easie= r in the long term. > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> Agreed, too. But again, this special problem appears from tim= e to time in the forums. > > >>>>>>>>>>=20 > > >>>>>>>>>> Why do we not have a ticket on BZ then? > > >>>>>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> The ticket is there ( see subject of this discussion ) and the = solution exists. > > >>>>>>>>=20 > > >>>>>>>> I disagree with this =E2=80=9Csolution=E2=80=9D. > > >>>>>>>>=20 > > >>>>>>>=20 > > >>>>>>> Why? > > >>>>>>>=20 > > >>>>>>> -Bernhard > > >>>>>>>=20 > > >>>>>>>>>=20 > > >>>>>>>>> -Bernhard > > >>>>>>>>>=20 > > >>>>>>>>>>> It is no problem, to describe the behaviour each time ( defin= e fixed lease -> press 'add' -> press 'update' --> definition is stored ), bu= t what about a easy solution, which deletes this discussion? Exactly this was= the reason for the patch. The amount of modified code isn't so big to demand= the canonical development process, IMHO. The patch can be applied by any cor= e developper reading DevList/bugzilla/forums regulary. > > >>>>>>>>>>=20 > > >>>>>>>>>> It is entirely up to you how you develop your code. How it is = submitted to the list is clear. > > >>>>>>>>>>=20 > > >>>>>>>>>> -Michael > > >>>>>>>>>>=20 > > >>>>>>>>>>>=20 > > >>>>>>>>>>> -Bernhard > > >>>>>>>>>>>=20 > > >>>>>>>>>>>> -Michael > > >>>>>>>>>>>>=20 > > >>>>>>>>>>>>>=20 > > >>>>>>>>>>>>> Regards, > > >>>>>>>>>>>>> Bernhard > >=20 > > > --===============8881584208274850393==--