From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click Date: Fri, 17 May 2019 20:18:36 +0100 Message-ID: <4BCAD6B4-E969-4EDE-A988-272154B6259D@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6729395259485858314==" List-Id: --===============6729395259485858314== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, This bug is currently assigned to Florian, because I have asked him to have a= look at it. I do not really care who is working on this, but I would like everyone to wor= k together on it. I would also like to stress that we have urgent fixes for loads of Intel proc= essors 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. Best, -Michael > 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 s= olve the problem, maybe it introduced some new problems. Sorry for my quick a= nd dirty reaction. >=20 > - I wasn't satisfied with the situation being. The problem exists furthermo= re 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 short 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 ) should an= d can be optimised. > The operations "add new fixed lease", "edit existing fixed lease", "add dyn= amic 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 leases lis= t, sort according the existing order. It should not be necessary to edit it f= irst. > "edit existing fixed lease": move parameters of selected entry to edit 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 bo= x. Editing is mandatory! We should not merge sets of fixed and dynamic 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 result. > Because of the number of changes in source ( comments, enhancements for rea= dability, .... ) I would suggest a commit of approved file as a whole, not ju= st as single little patches. How can this be accomplished? > I think little patches here and there cannot solve the problem of low maint= ainability 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 priva= te, telling me that he no longer wants to pursue getting this patch merged. >>=20 >> I find this whole situation very frustrating, but of course I accept his d= ecision. >>=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 wrot= e: >>>=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 with one 'a= dd' click >>>>=20 >>>> Hi, >>>>=20 >>>>> On 18 Apr 2019, at 13:54, Bernhard Bitsch wr= ote: >>>>>=20 >>>>> Hello, >>>>>=20 >>>>> I do not want to start any discussions about the way the project does i= ts 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 with 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 on= e '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 leases with = one 'add' click >>>>>>>>>>=20 >>>>>>>>>> Hi, >>>>>>>>>>=20 >>>>>>>>>> Thanks Matthias for helping out here. However, I do not get the pa= tch. >>>>>>>>>>=20 >>>>>>>>>> There is no explanation about what it is meant to do. The intentio= n 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 wit= h 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 thousands = 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 pag= e, this wasn't my main effort for this quick and short solution. For this spe= cial case I regarded the forum post and the bugzilla entry to be sufficient d= ocumentation, 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 $Lang::tr{'a= dd'}.'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 comp= lete because >>>>>>>>>>> # if ip are not inside a known subnet, I don't warn. >>>>>>>>>>> # Also it may be needed to put duplicate fixed lease in thei= r right subnet definition.. >>>>>>>>>>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($d= hcpsettings{'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 n= eed 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 $Lang::tr{'a= dd'}.'2') { >>>>>>>>>>> $dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dhcpsettin= gs{'FIX_FILENAME'}); >>>>>>>>>>> $dhcpsettings{'FIX_ROOTPATH'} =3D &Header::cleanhtml($dhcpsettin= gs{'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 should no= t make it worse either. >>>>>>>>>>=20 >>>>>>>>> How should indention be done? By spaces, tabs? How many colums defi= ne an indent? >>>>>>>>=20 >>>>>>>> In the patch it looks like you wrapped the block into another if con= dition. So it has to be indented to make that obvious. >>>>>>>>=20 >>>>>>>=20 >>>>>>> Maybe this generated by different editors, I used. Indention of the p= atch isn't worse than the existing indentation. Not being a final version ( s= ee above ) it is sufficient, in my opinion. >>>>>>=20 >>>>>> Do *not* submit non-final versions. There is no point in it. A patch i= s meant to go into a final release. We do not want beta stuff in there. >>>>>>=20 >>>>>=20 >>>>> Okay. Got it. I should have sent my modified dhcp.cgi because of lack o= f a working git repo on my site. >>>>=20 >>>> Please learn how to use git and use =E2=80=9Cgit send-email=E2=80=9D. Th= ere is an introduction on the wiki on how to set it up and there is tons of r= esources on the Internet that explain Git to you in the form of howtos and vi= deos. >>>>=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 thin= k. On the other hand I've tried to help with limited sources and without mone= tary assistance, till now. If this isn't possible, it is okay for me. I'll re= tire 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" definiti= on, which I didn't find in the docs (yet). >>>>>>> I'll use this definition for further development ( and formated 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 fre= e 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 spaces. The= refore my statement. If we commit us to tab=3D4 these spaces can be eliminate= d. >>>>=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 particular file.= Do not re-indent the whole file. >>>>=20 >>>=20 >>> If that's the opinion of the majority of the developers, live with it. On= e reason of the ending of IPCop development was this rigid interpretation 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 spacing is= n't a difference" option. >>>>>>>>=20 >>>>>>>> Please just use =E2=80=9Cgit diff=E2=80=9D. It has a variety of sett= ings that are just right. >>>>>>>>=20 >>>>>>>=20 >>>>>>> I'll use this in future. Because I didn't do the modification in a gi= t repo, but in the working system, I didn't realize the possibility of diffin= g two arbitrary files with "git diff". Sorry. >>>>>>=20 >>>>>> Of course you would have the files in your working system. How else wo= uld you test? >>>>>>=20 >>>>>> But for development purposes git is being used. It is the standard. Ot= herwise patches won=E2=80=99t apply. There is no point in sending patches tha= t other developers cannot apply. Use Git. >>>>>>=20 >>>>>=20 >>>>> Ok. If I don't have a git repo myself, I send the modified source. 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 not work. = Of course you could send instructions to other people on how to change files,= but that doesn=E2=80=99t work either. >>>>=20 >>>> I personally won=E2=80=99t do any work of trying to apply any patches th= at are send in other forms. There is also reasons that we send patches 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'},$dhcpsett= ings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$= dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'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=3Dcommitdiff;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 commentation. ;) >>>>>>=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 th= e leases file is sorted, the key changes but is not updated. That lead 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, to= o. >>>>=20 >>>=20 >>> I know from reading and understandig the code, that this works. The appro= val 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'},$dhcpsettings{= 'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcps= ettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMA= RK'}\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'},$dhcpsettings{'FIX_= NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcps= ettings{'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 behaviour mentio= ned 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 only for y= ou, but generally: >>>>>>>>=20 >>>>>>>> I am not interested in quick and dirty solutions. That is how you br= eak things. I am interested in well-documented, peer-reviewed and tested code= . We are creating some piece of high-quality software - or aim to do so at le= ast - and there is no space for quick and dirty. >>>>>>>=20 >>>>>>> This special modication wasn't intended as quick and dirty, but as qu= ick and stable and intuitive. Sorry, the first version was really a 'dirty sh= ot'. But I wanted to present this as soon as possible for review and test, wh= ich was done by Matthias Fischer.=20 >>>>>>> I agree fully with your aim, but this means readability also! This me= ans some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted? >>>>>>=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 and there= are open questions. Why would we merge something that clearly adds more prob= lems 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 opinion about = the fixed problem. >>>=20 >>>>>>>> Please figure out how to set up a local Git repository, how to set u= p a branch, how to commit things and what rules there are to follow. Then fin= d out on how to submit a patchset to the mailing list - after it has been tes= ted. It is all in the link that I sent you. >>>>>>>>=20 >>>>>>>> This is not a competition about who can submit patches the fastest. >>>>>>>>=20 >>>>>>>=20 >>>>>>> As stated before, the fact I didn't use a git repo has reasons in my = personal installation at the moment. Therefore your reminder about that isn't= relevant for this single case. I also stated, that I'll surely use git for g= reater modifications I do on this topic ( and others ). >>>>>>> I didn't want to start a competition. It was just one more post in th= e forum about not intuitive behaviour of the DHCP WUI page, resulting in a bu= gzilla 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 example, m= aybe this is invoked now by my modification). >>>>>>>>=20 >>>>>>>> What are those? Why are we not talking about those first and then co= me 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 possible and= I am just working on this. I'll discuss these topics in the list before send= ing patches, being aware this is a community project this many opinions. >>>>>>=20 >>>>>> This is not necessarily about opinions. It is about what we can suppor= t 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 t= ime also. In the moment I do this mainly. >>>>>=20 >>>>>> Think about that we are all in the same boat and we want to improve IP= Fire wherever we can. But we need to talk about things because one set of eye= s often is not enough. Something that works for one person does not work for = another. We can never break backwards-compatibility. >>>>>>=20 >>>>>> I personally want to see this static lease bug fixed. I am trying to h= elp 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 some long= time ago and that work for this project. That way we will hopefully all bene= fit from this. >>>>>>=20 >>>>>=20 >>>>> Why didn't you just say this. A request for the whole file would have b= een 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 about why t= he 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 deb= ating 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. Again, 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 future then. >>>>=20 >>>=20 >>> There is no need to behave like this. I'll be quiet myself upon source qu= ality and proposed fixes for bugs. >>>=20 >>>>>>>>> I am reviewing this piece of code at the moment and plan to sugges= t 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 reason. This= is not to make things difficult. In fact it makes things a lot easier in the= long term. >>>>>>>>=20 >>>>>>>=20 >>>>>>> Agreed, too. But again, this special problem appears from time to tim= e 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 ( define fixed = lease -> press 'add' -> press 'update' --> definition is stored ), but what a= bout a easy solution, which deletes this discussion? Exactly this was the rea= son for the patch. The amount of modified code isn't so big to demand the can= onical development process, IMHO. The patch can be applied by any core develo= pper reading DevList/bugzilla/forums regulary. >>>>>>=20 >>>>>> It is entirely up to you how you develop your code. How it is submitte= d to the list is clear. >>>>>>=20 >>>>>> -Michael >>>>>>=20 >>>>>>>=20 >>>>>>> -Bernhard >>>>>>>=20 >>>>>>>> -Michael >>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> Regards, >>>>>>>>> Bernhard >>=20 >>=20 --===============6729395259485858314==--