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: Sat, 20 Apr 2019 17:35:04 +0100 Message-ID: <39475594-693C-40AD-9874-C20B726E4086@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1115354725944214378==" List-Id: --===============1115354725944214378== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, 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 merged. I find this whole situation very frustrating, but of course I accept his deci= sion. I guess we just have to agree that we disagree here. Best, -Michael > 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 with one 'add= ' click >>=20 >> Hi, >>=20 >>> On 18 Apr 2019, at 13:54, Bernhard Bitsch wrot= e: >>>=20 >>> Hello, >>>=20 >>> I do not want to start any discussions about the way the project 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 with one 'a= dd' click >>>>=20 >>>> Hello, >>>>=20 >>>>> On 18 Apr 2019, at 12:23, Bernhard Bitsch wr= ote: >>>>>=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 leases with on= e 'add' click >>>>>>>>=20 >>>>>>>> Hi, >>>>>>>>=20 >>>>>>>> Thanks Matthias for helping out here. However, I do not get the patc= h. >>>>>>>>=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 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 page,= this wasn't my main effort for this quick and short solution. For this speci= al case I regarded the forum post and the bugzilla entry to be sufficient doc= umentation, 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{'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 comple= te 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($dhc= psettings{'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 nee= d 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{'add= '}.'2') { >>>>>>>>> $dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dhcpsettings= {'FIX_FILENAME'}); >>>>>>>>> $dhcpsettings{'FIX_ROOTPATH'} =3D &Header::cleanhtml($dhcpsettings= {'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 not = make it worse either. >>>>>>>>=20 >>>>>>> How should indention be done? By spaces, tabs? How many colums define= an indent? >>>>>>=20 >>>>>> In the patch it looks like you wrapped the block into another if condi= tion. So it has to be indented to make that obvious. >>>>>>=20 >>>>>=20 >>>>> Maybe this generated by different editors, I used. Indention of the pat= ch isn't worse than the existing indentation. Not being a final version ( 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 there. >>>>=20 >>>=20 >>> Okay. Got it. I should have sent my modified dhcp.cgi because of 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. Ther= e is an introduction on the wiki on how to set it up and there is tons of res= ources on the Internet that explain Git to you in the form of howtos and vide= os. >>=20 >=20 > Sorry, I know about git. It is not the lack of knowledge, but my limited eq= uipment. 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 without moneta= ry assistance, till now. If this isn't possible, it is okay for me. I'll reti= re to the status "consuming user". >=20 >>>>>=20 >>>>>>> The original file (and many ohters!) have an mixture of tab/space. Sh= ould 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 formated dhcpi.c= gi in my work copy in the neighbourhood of the change, yet ). >>>>> This means that further patches may contain such "cosmetics", if they a= re 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 spaces. There= fore my statement. If we commit us to tab=3D4 these spaces can be eliminated. >>=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. D= o not re-indent the whole file. >>=20 >=20 > If that's the opinion of the majority of the developers, live with it. One = reason of the ending of IPCop development was this rigid interpretation of "d= on't touch a running system", IMHO. >=20 >=20 >>>>>=20 >>>>>>> No problem. But this will produce a great number of patches. I made t= hese modifications local, but generated the diff with "different spacing isn'= t a difference" option. >>>>>>=20 >>>>>> Please just use =E2=80=9Cgit diff=E2=80=9D. It has a variety of settin= gs 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 of diffing = two arbitrary files with "git diff". Sorry. >>>>=20 >>>> Of course you would have the files in your working system. How else woul= d you test? >>>>=20 >>>> But for development purposes git is being used. It is the standard. Othe= rwise patches won=E2=80=99t apply. There is no point in sending patches that = other developers cannot apply. Use Git. >>>>=20 >>>=20 >>> Ok. If I don't have a git repo myself, I send the modified source. Each d= eveloper 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, b= ut that doesn=E2=80=99t work either. >>=20 >> I personally won=E2=80=99t do any work of trying to apply any patches that= 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'},$dhcpsettin= gs{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dh= cpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_R= EMARK'}\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=3D3= 1672dc8bdb223ebf425ff96be64318f2d68e0d7 >>>>>>>>=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 the = leases file is sorted, the key changes but is not updated. That lead to the c= ase 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. The approva= l 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{'F= IX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpset= tings{'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'},$dhcpsettings{'FIX_NE= XTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpset= tings{'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 mentione= d 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 you= , but generally: >>>>>>=20 >>>>>> I am not interested in quick and dirty solutions. That is how you brea= k 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 leas= t - and there is no space for quick and dirty. >>>>>=20 >>>>> This special modication wasn't intended as quick and dirty, but as quic= k 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, whic= h was done by Matthias Fischer.=20 >>>>> I agree fully with your aim, but this means readability also! This mean= s 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 p= romise to fix it later. There are obvious problems with the patch and there a= re open questions. Why would we merge something that clearly adds more proble= ms 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 th= e 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 teste= d. 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 pe= rsonal installation at the moment. Therefore your reminder about that isn't r= elevant for this single case. I also stated, that I'll surely use git for gre= ater modifications I do on this topic ( and others ). >>>>> I didn't want to start a competition. It was just one more post in the = forum about not intuitive behaviour of the DHCP WUI page, resulting in a bugz= illa 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, may= be 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 b= e 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 sendin= g patches, being aware this is a community project this many opinions. >>>>=20 >>>> This is not necessarily about opinions. It is about what we can 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 tim= e also. In the moment I do this mainly. >>>=20 >>>> Think about that we are all in the same boat and we want to improve IPFi= re wherever we can. But we need to talk about things because one set of eyes = often is not enough. Something that works for one person does not work for an= other. We can never break backwards-compatibility. >>>>=20 >>>> I personally want to see this static lease bug fixed. I am trying to hel= p you to develop a good solution that we do not have to worry about in the fu= ture. I am also enforcing the rules that we all have come up with some long t= ime ago and that work for this project. That way we will hopefully all benefi= t from this. >>>>=20 >>>=20 >>> Why didn't you just say this. A request for the whole file would have bee= n 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 the= rules are as they are. If there is a constructive proposal to make things ea= sier for all then we are all of course open for this. On the other hand debat= ing what is the standard now is just a waste of time. I am happy to explain t= his, but I am not willing to debate them or compromise on this. Again, this i= s 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 al= so 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 qual= ity and proposed fixes for bugs. >=20 >>>>>>> I am reviewing this piece of code at the moment and plan to 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 reason. This i= s not to make things difficult. In fact it makes things a lot easier in the l= ong term. >>>>>>=20 >>>>>=20 >>>>> Agreed, too. But again, this special problem appears from time 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 e= xists. >>=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 le= ase -> press 'add' -> press 'update' --> definition is stored ), but what abo= ut a easy solution, which deletes this discussion? Exactly this was the reaso= n for the patch. The amount of modified code isn't so big to demand the canon= ical development process, IMHO. The patch can be applied by any core developp= er 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 --===============1115354725944214378==--