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: Thu, 18 Apr 2019 14:54:12 +0200 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0957597504182263987==" List-Id: --===============0957597504182263987== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, I do not want to start any discussions about the way the project does its wor= k. Therefore some (hopefully) short annotations below. > 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 > > 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 mea= nt to go into a final release. We do not want beta stuff in there. >=20 Okay. Got it. I should have sent my modified dhcp.cgi because of lack of a wo= rking git repo on my site. > >=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 > Especially in this file I found a messy mixture of tabs and spaces. Therefore= my statement. If we commit us to tab=3D4 these spaces can be eliminated. =20 > >=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 y= ou test? >=20 > But for development purposes git is being used. It is the standard. Otherwi= se patches won=E2=80=99t apply. There is no point in sending patches that oth= er developers cannot apply. Use Git. >=20 Ok. If I don't have a git repo myself, I send the modified source. Each devel= oper with an actual git repo can apply it by commit. Right? > >=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? > Ok. I ask you now. Why doesn't it function? =20 > >=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'},$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 prom= ise 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 Where are the problems? Be more specific please. > >> 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 Helping users with known problems, which could be resolved, is wating time al= so. In the moment I do this mainly. > 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 oft= en is not enough. Something that works for one person does not work for anoth= er. We can never break backwards-compatibility. >=20 > I personally want to see this static lease bug fixed. I am trying to help y= ou to develop a good solution that we do not have to worry about in the futur= e. 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 benefit f= rom this. >=20 Why didn't you just say this. A request for the whole file would have been en= ough. BTW the file can be found in the forum post. > But I do not want to have endless discussions on this list about why the ru= les are as they are. If there is a constructive proposal to make things easie= r 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. Again, this is a= general statement and not limited to this conversation. >=20 You started this discussion ( once more ). > >=20 > >>> 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? > The ticket is there ( see subject of this discussion ) and the solution exist= s. -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 >=20 > --===============0957597504182263987==--