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: Thu, 18 Apr 2019 15:47:01 +0100 Message-ID: <93ABF8AC-046C-4B21-922F-8029291F9803@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0168588032325493832==" List-Id: --===============0168588032325493832== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, > 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 project does its w= ork. > 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 wrot= e: >>>=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 'a= dd' click >>>>=20 >>>> Hi, >>>>=20 >>>>> On 17 Apr 2019, at 22:49, Bernhard Bitsch wr= ote: >>>>>=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 patch. >>>>>>=20 >>>>>> There is no explanation about what it is meant to do. The intention al= ready 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 al= l 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 e= mails 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, t= his 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 sufficient docum= entation, 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 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 ri= ght subnet definition.. >>>>>>> + if ((lc($dhcpsettings{'FIX_MAC'}) eq lc($temp[0])) &&(lc($dhcps= ettings{'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 $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 ma= ke it worse either. >>>>>>=20 >>>>> How should indention be done? By spaces, tabs? How many colums define a= n indent? >>>>=20 >>>> In the patch it looks like you wrapped the block into another if conditi= on. 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 version ( see a= bove ) it is sufficient, in my opinion. >>=20 >> Do *not* submit non-final versions. There is no point in it. A patch is me= ant 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. Please learn how to use git and use =E2=80=9Cgit send-email=E2=80=9D. There i= s an introduction on the wiki on how to set it up and there is tons of resour= ces on the Internet that explain Git to you in the form of howtos and videos. >>>=20 >>>>> The original file (and many ohters!) have an mixture of tab/space. Shou= ld 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.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 spaces. Therefo= re my statement. If we commit us to tab=3D4 these spaces can be eliminated. We inherited some code from IPCop that we did not clean up. So the guideline is to use the coding style used in the particular file. Do n= ot re-indent the whole file. >>>=20 >>>>> No problem. But this will produce a great number of patches. I made the= se 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 settings= that are just right. >>>>=20 >>>=20 >>> I'll use this in future. Because I didn't do the modification in a git re= po, but in the working system, I didn't realize the possibility of diffing tw= o 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 standard. Otherw= ise patches won=E2=80=99t apply. There is no point in sending patches that ot= her developers cannot apply. Use Git. >>=20 >=20 > Ok. If I don't have a git repo myself, I send the modified source. Each dev= eloper with an actual git repo can apply it by commit. Right? Please learn how to use git. Of course you could send modified files around, but that does not work. Of co= urse you could send instructions to other people on how to change files, but = that doesn=E2=80=99t work either. I personally won=E2=80=99t do any work of trying to apply any patches that ar= e send in other forms. There is also reasons that we send patches inline: We = can comment on them. Please learn how to use git. >>>=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_NEXTADDR'},$dhcp= settings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REM= ARK'}\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=3D316= 72dc8bdb223ebf425ff96be64318f2d68e0d7 >>>>>>=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? Well, as the patch there states the key is saved and used later. When the lea= ses file is sorted, the key changes but is not updated. That lead to the case= that you edited a different lease than you wanted. I did not test this, but I could imagine that this could happen here, too. >>>>=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'},$dhcpsetti= ngs{'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_NEXT= ADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsetti= ngs{'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 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 only for you, = but generally: >>>>=20 >>>> I am not interested in quick and dirty solutions. That is how you break = 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 least = - and there is no space for quick and dirty. >>>=20 >>> This special modication wasn't intended as quick and dirty, but 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 accepted? >>=20 >> You cannot submit a patch that does not fulfil the guidelines and then pro= mise 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 problems= than it solves? >>=20 >> So no, it is not accepted. >>=20 >=20 > Where are the problems? > Be more specific please. Read my first email on the patch. That is as specific as it gets. >>>> 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 ou= t 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 fastest. >>>>=20 >>>=20 >>> As stated before, the fact I didn't use a git repo has reasons in my pers= onal installation at the moment. Therefore your reminder about that isn't rel= evant for this single case. I also stated, that I'll surely use git for great= er modifications I do on this topic ( and others ). >>> I didn't want to start a competition. It was just one more post in the fo= rum about not intuitive behaviour of the DHCP WUI page, resulting in a bugzil= la topic by Matthias.=20 >>>=20 >>>>> I found a couple of behaviours of this page, which are not obvious or s= traight forward ( adding dynamic leases to static leases, for example, maybe= this is invoked now by my modification). >>>>=20 >>>> What are those? Why are we not talking about those first and then come u= p 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 a= m just working on this. I'll discuss these topics in the list before sending = 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 time = also. In the moment I do this mainly. >=20 >> Think about that we are all in the same boat and we want to improve IPFire= wherever we can. But we need to talk about things because one set of eyes of= ten is not enough. Something that works for one person does not work for anot= her. We can never break backwards-compatibility. >>=20 >> I personally want to see this static lease bug fixed. I am trying to help = you to develop a good solution that we do not have to worry about in the futu= re. I am also enforcing the rules that we all have come up with some long tim= e 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 would have been = enough. BTW the file can be found in the forum post. There are problems *in* the file. I raised questions. >> But I do not want to have endless discussions on this list about why the r= ules are as they are. If there is a constructive proposal to make things easi= er for all then we are all of course open for this. On the other hand debatin= g what is the standard now is just a waste of time. I am happy to explain thi= s, 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 ). Okay, I tried to be helpful here. If you prefer to insist that this patch is not being amended and if you also = prefer to point fingers, keep doing it. I will remember this and keep my comments to myself in the future then. >>>>> I am reviewing this piece of code at the moment and plan to suggest so= me modifications/corrections with formally legal patches. I hope this is ok f= or 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 lon= g 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 exi= sts. I disagree with this =E2=80=9Csolution=E2=80=9D. >=20 > -Bernhard >=20 >>> It is no problem, to describe the behaviour each time ( define fixed leas= e -> press 'add' -> press 'update' --> definition is stored ), but 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 canonic= al development process, IMHO. The patch can be applied by any core 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 --===============0168588032325493832==--