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: Mon, 20 May 2019 10:37:45 +0100 Message-ID: <4098F77F-04C7-4176-9502-DF934425CA2E@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6436027868521072588==" List-Id: --===============6436027868521072588== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, > On 18 May 2019, at 19:28, Bernhard Bitsch wrote: >=20 > Hi, >=20 >> Gesendet: Samstag, 18. Mai 2019 um 00:27 Uhr >> Von: "Michael Tremer" >> An: "Bernhard Bitsch" >> Cc: "IPFire: Development-List" , florian.b= uehrle(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 wrot= e: >>>=20 >>> Hi, >>>=20 >>> sorry if I were not exact enough. I am working with a matter of urgency o= n 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. I doubt that. This is an Open Source project, we do things =E2=80=9Cin the op= en=E2=80=9D. https://blog.ipfire.org/post/default-to-open-please >=20 >>> My post was just a heads up about this work. The rewriting will be the se= cond step, after bug elimination. The purpose should be to faciliate future b= ug fixes. >>=20 >> I do not think that it makes any sense to rewrite this CGI file. We are go= ing to get rid of the whole web UI in IPFire 3 and there are no new features = to expect in the DHCP area that it makes sense to tidy up to much. I am not a= dvocating to have messy code here, but it works, and it easily breaks if you = touch something. >>=20 >=20 > When will IPFire 3 come? That is unknown. But every minute spend on IPFire 2 will delay it by another = minute. > How should we find and correct errors in the meantime?=20 > About breaking by touching this messy code, I agree with you. But if the me= ss remains, the probability of importing new errors by "bug fixes" increases.= Just my opinion. >=20 >>> My idea for quick inclusion of fix: >>> Florian is working on it, therefore I'll discuss possible solutions with = him. He will commit these to the git repo, based on the actual dhcp.cgi file. >>=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 publis= h 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 sending i= n patches that clean up code but do not change any behaviour you should colle= ct them in a git branch and send that branch as a patchset. Bug fixes should = be sent separately if it makes sense. >>=20 >=20 > Okay, that is the way I thought of. At the moment Florian fixes the actual= error in the existing code. Independend of that I represent the rewrite in g= it 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; yet: > - Adding a new fixed lease adds this directly, without having to click a se= cond time. That is already the case. > - Adding a dynamic lease to the fixed leases should work in two steps: firs= t the data from dynamic leases is copied to the edit fields, user can change = and complete the definition and adds this by clicking "add". A check for disj= unction of sets of fixed and dynamic leases would be possible. If you add it from the DHCP leases list, the static lease is meant to be adde= d right away, but you can still edit it to give it a better name or so. This = what currently seems to be broken. So you are not talking about changing any behaviour. This *is* the bug we are= talking about all along. -Michael >=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" , florian= .buehrle(a)ipfire.org >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'a= dd' click >>>>=20 >>>> Hi, >>>>=20 >>>> This bug is currently assigned to Florian, because I have asked him to h= ave a look at it. >>>>=20 >>>> I do not really care who is working on this, but I would like everyone t= o work together on it. >>>>=20 >>>> I would also like to stress that we have urgent fixes for loads of Intel= processors in Core Update 132 and I think that this problem should also be f= ixed 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 wr= ote: >>>>>=20 >>>>> Hello, >>>>>=20 >>>>> just some news about this topic. >>>>>=20 >>>>> - Michael was right to refuse my quick and dirty patch. It did not real= ly solve the problem, maybe it introduced some new problems. Sorry for my qui= ck and dirty reaction. >>>>>=20 >>>>> - I wasn't satisfied with the situation being. The problem exists furth= ermore 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 ) shoul= d 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 leases= list, sort according the existing order. It should not be necessary to edit = it first. >>>>> "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 edi= t box. Editing is mandatory! We should not merge sets of fixed and dynamic le= ases. 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= readability, .... ) I would suggest a commit of approved file as a whole, no= t just as single little patches. How can this be accomplished? >>>>> I think little patches here and there cannot solve the problem of low m= aintainability 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 p= rivate, telling me that he no longer wants to pursue getting this patch merge= d. >>>>>>=20 >>>>>> I find this whole situation very frustrating, but of course I accept h= is 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 with on= e '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 project do= es 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 '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 wit= h 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 w= ith one 'add' click >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> Thanks Matthias for helping out here. However, I do not get th= e patch. >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> There is no explanation about what it is meant to do. The inte= ntion 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 thousa= nds 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 sufficie= nt 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 $Lang::t= r{'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])) &&(l= c($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 $Lang::t= r{'add'}.'2') { >>>>>>>>>>>>>>> $dhcpsettings{'FIX_FILENAME'} =3D &Header::cleanhtml($dhcpse= ttings{'FIX_FILENAME'}); >>>>>>>>>>>>>>> $dhcpsettings{'FIX_ROOTPATH'} =3D &Header::cleanhtml($dhcpse= ttings{'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 shoul= d 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= condition. So it has to be indented to make that obvious. >>>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>> Maybe this generated by different editors, I used. Indention of t= he patch 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 pat= ch 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 la= ck 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 howtos an= d videos. >>>>>>>>=20 >>>>>>>=20 >>>>>>> Sorry, I know about git. It is not the lack of knowledge, but my limi= ted 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 without = monetary assistance, till now. If this isn't possible, it is okay for me. I'l= l retire to the status "consuming user". >>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>>> The original file (and many ohters!) have an mixture of tab/spa= ce. 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" defi= nition, which I didn't find in the docs (yet). >>>>>>>>>>> I'll use this definition for further development ( and formated d= hcpi.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.= Therefore my statement. If we commit us to tab=3D4 these spaces can be elimi= nated. >>>>>>>>=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 f= ile. Do 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 "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 spacin= g 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 of di= ffing two arbitrary files with "git diff". Sorry. >>>>>>>>>>=20 >>>>>>>>>> Of course you would have the files in your working system. How els= e would you test? >>>>>>>>>>=20 >>>>>>>>>> But for development purposes git is being used. It is the standard= . Otherwise 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 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 wo= rk. Of course you could send instructions to other people on how to change fi= les, but that doesn=E2=80=99t work either. >>>>>>>>=20 >>>>>>>> I personally won=E2=80=99t do any work of trying to apply any patche= s that are send in other forms. There is also reasons that we send patches in= line: 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'},$dhcp= settings{'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. Whe= n the 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= , too. >>>>>>>>=20 >>>>>>>=20 >>>>>>> I know from reading and understandig the code, that this works. The a= pproval by Matthias Fischer shows, that I'm right. See the related forum thre= ad. >>>>>>> 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'},$dhcpsetti= ngs{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$d= hcpsettings{'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_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$d= hcpsettings{'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 me= ntioned 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 f= or you, but generally: >>>>>>>>>>>>=20 >>>>>>>>>>>> I am not interested in quick and dirty solutions. That is how yo= u 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 a= t least - and there is no space for quick and dirty. >>>>>>>>>>>=20 >>>>>>>>>>> This special modication wasn't intended as quick and dirty, but a= s quick and stable and intuitive. Sorry, the first version was really a 'dirt= y 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! Thi= s means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accept= ed? >>>>>>>>>>=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 t= here 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. >>>>>>>>=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 ab= out the fixed problem. >>>>>>>=20 >>>>>>>>>>>> Please figure out how to set up a local Git repository, how to s= et 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 faste= st. >>>>>>>>>>>>=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 i= sn't relevant for this single case. I also stated, that I'll surely use git f= or greater modifications I do on this topic ( and others ). >>>>>>>>>>> I didn't want to start a competition. It was just one more post i= n 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 obvi= ous or straight forward ( adding dynamic leases to static leases, for exampl= e, maybe this is invoked now by my modification). >>>>>>>>>>>>=20 >>>>>>>>>>>> What are those? Why are we not talking about those first and the= n 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 possible= and I am 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 su= pport in the end and where we all want to invest our time. >>>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> Helping users with known problems, which could be resolved, is wati= ng time also. In the moment I do this mainly. >>>>>>>>>=20 >>>>>>>>>> Think about that we are all in the same boat and we want to improv= e IPFire 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 another. 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 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 = benefit from this. >>>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>> Why didn't you just say this. A request for the whole file would ha= ve 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 about w= hy the rules are as they are. If there is a constructive proposal to make thi= ngs 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 exp= lain 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 th= en. >>>>>>>>=20 >>>>>>>=20 >>>>>>> There is no need to behave like this. I'll be quiet myself upon sourc= e quality and proposed fixes for bugs. >>>>>>>=20 >>>>>>>>>>>>> I am reviewing this piece of code at the moment and plan to su= ggest 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= 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 solu= tion 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 fi= xed lease -> press 'add' -> press 'update' --> definition is stored ), but wh= at 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 core de= velopper reading DevList/bugzilla/forums regulary. >>>>>>>>>>=20 >>>>>>>>>> It is entirely up to you how you develop your code. How it is subm= itted to the list is clear. >>>>>>>>>>=20 >>>>>>>>>> -Michael >>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>> -Bernhard >>>>>>>>>>>=20 >>>>>>>>>>>> -Michael >>>>>>>>>>>>=20 >>>>>>>>>>>>>=20 >>>>>>>>>>>>> Regards, >>>>>>>>>>>>> Bernhard --===============6436027868521072588==--