Hi,
On 17 May 2019, at 23:21, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:
Hi,
sorry if I were not exact enough. I am working with a matter of urgency on the DHCP config problem and surely will contact Florian, if I've found the error and a real fix for it.
No need to contact him directly. That is what this list is for.
My post was just a heads up about this work. The rewriting will be the second step, after bug elimination. The purpose should be to faciliate future bug fixes.
I do not think that it makes any sense to rewrite this CGI file. We are going 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 advocating to have messy code here, but it works, and it easily breaks if you touch something.
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.
Why don’t you say it here what you have found?
When I am finished with the rewrite (including the error fix) I'll publish it either here in the devel list or in git or both. Is this ok?
That really depends on what you want to achieve here. If you are sending in patches that clean up code but do not change any behaviour you should collect them in a git branch and send that branch as a patchset. Bug fixes should be sent separately if it makes sense.
If you plan to change any behaviour of the CGI file, that is a matter open for discussion first and then work should start.
Hope this makes sense.
-Michael
Best, Bernhard
Gesendet: Freitag, 17. Mai 2019 um 21:18 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org, florian.buehrle@ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
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 work together on it.
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 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 Bernhard.Bitsch@gmx.de wrote:
Hello,
just some news about this topic.
Michael was right to refuse my quick and dirty patch. It did not really solve the problem, maybe it introduced some new problems. Sorry for my quick and dirty reaction.
I wasn't satisfied with the situation being. The problem exists furthermore 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 ).
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 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.
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 edit box. Editing is mandatory! We should not merge sets of fixed and dynamic leases. Add new entry as new fixed lease with changed parameters, sort.
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, not just as single little patches. How can this be accomplished? I think little patches here and there cannot solve the problem of low maintainability of this file.
Regards, Bernhard
Gesendet: Samstag, 20. April 2019 um 18:35 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
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 decision.
I guess we just have to agree that we disagree here.
Best, -Michael
On 18 Apr 2019, at 21:37, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:
Gesendet: Donnerstag, 18. April 2019 um 16:47 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de Cc: "IPFire: Development-List" development@lists.ipfire.org Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click
Hi,
> On 18 Apr 2019, at 13:54, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: > > Hello, > > I do not want to start any discussions about the way the project does its work. > Therefore some (hopefully) short annotations below. > >> Gesendet: Donnerstag, 18. April 2019 um 13:42 Uhr >> Von: "Michael Tremer" michael.tremer@ipfire.org >> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >> Cc: "IPFire: Development-List" development@lists.ipfire.org >> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >> >> Hello, >> >>> On 18 Apr 2019, at 12:23, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>> >>> Hi, >>> >>>> Gesendet: Donnerstag, 18. April 2019 um 11:50 Uhr >>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>> An: "Bernhard Bitsch" Bernhard.Bitsch@gmx.de >>>> Cc: BeBiMa bbitsch@ipfire.org, "IPFire: Development-List" development@lists.ipfire.org >>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>> >>>> Hi, >>>> >>>>> On 17 Apr 2019, at 22:49, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote: >>>>> >>>>> Hi, >>>>> some explanations from the author: >>>>> >>>>>> Gesendet: Mittwoch, 17. April 2019 um 11:31 Uhr >>>>>> Von: "Michael Tremer" michael.tremer@ipfire.org >>>>>> An: "Matthias Fischer" matthias.fischer@ipfire.org >>>>>> Cc: development@lists.ipfire.org, BeBiMa bbitsch@ipfire.org >>>>>> Betreff: Re: [PATCH] Fix for Bug #12050: Adding fixed leases with one 'add' click >>>>>> >>>>>> Hi, >>>>>> >>>>>> Thanks Matthias for helping out here. However, I do not get the patch. >>>>>> >>>>>> There is no explanation about what it is meant to do. The intention already is that the lease is added in the first place. >>>>>> >>>>> >>>>> 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. >>>> >>>> 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. >>>> >>> >>> 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 sufficient documentation, for the moment. >> >> No. >> >> Every commit must contain itself. >> >>>>> >>>>>>> On 16 Apr 2019, at 17:41, Matthias Fischer matthias.fischer@ipfire.org wrote: >>>>>>> >>>>>>> Signed-off-by: BeBiMa bbitsch@ipfire.org >>>>>>> Reviewed-by: Matthias Fischer matthias.fischer@ipfire.org >>>>>>> --- >>>>>>> html/cgi-bin/dhcp.cgi | 12 ++++++++++++ >>>>>>> 1 file changed, 12 insertions(+) >>>>>>> >>>>>>> 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') { >>>>>>> } >>>>>>> >>>>>>> my $key = 0; >>>>>>> + my $szc = scalar(@current2); >>>>>>> CHECK:foreach my $line (@current2) { >>>>>>> my @temp = 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])) &&(lc($dhcpsettings{'FIX_ADDR'}) eq lc($temp[1]))) { >>>>>>> + last CHECK; >>>>>>> + } >>>>>> >>>>>> Why is this needed? >>>>> >>>>> Check for existing lease. If <MAC,IP> is defined already we don't need to loop further. >>>>> >>>>>> >>>>>>> foreach my $itf (@ITFs) { >>>>>>> my $scoped = &General::IpInSubnet($dhcpsettings{'FIX_ADDR'}, >>>>>>> $netsettings{"${itf}_NETADDRESS"}, >>>>>>> @@ -442,11 +446,19 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') { >>>>>>> $dhcpsettings{'FIX_FILENAME'} = &Header::cleanhtml($dhcpsettings{'FIX_FILENAME'}); >>>>>>> $dhcpsettings{'FIX_ROOTPATH'} = &Header::cleanhtml($dhcpsettings{'FIX_ROOTPATH'}); >>>>>>> if ($dhcpsettings{'KEY2'} eq '') { #add or edit ? >>>>>> >>>>>> This block here is not indented correctly. >>>>>> >>>>>> I understand that the code is already very messy, but we should not make it worse either. >>>>>> >>>>> How should indention be done? By spaces, tabs? How many colums define an indent? >>>> >>>> In the patch it looks like you wrapped the block into another if condition. So it has to be indented to make that obvious. >>>> >>> >>> 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 above ) it is sufficient, in my opinion. >> >> 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. >> > > 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 “git send-email”. 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 and videos.
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 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'll retire to the status "consuming user".
>>> >>>>> The original file (and many ohters!) have an mixture of tab/space. Should we patch that step by step ( tab=4 ) to increase readability. >>>> >>>> This is not about tabs or spaces. >>>> >>> >>> Sure! This mixture doesnt't matter only in case of "tab=4" 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. >> >> 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. >> >> I liked you the coding style. >> > > Especially in this file I found a messy mixture of tabs and spaces. Therefore my statement. If we commit us to tab=4 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 not re-indent the whole file.
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.
>>> >>>>> No problem. But this will produce a great number of patches. I made these modifications local, but generated the diff with "different spacing isn't a difference" option. >>>> >>>> Please just use “git diff”. It has a variety of settings that are just right. >>>> >>> >>> 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. >> >> Of course you would have the files in your working system. How else would you test? >> >> But for development purposes git is being used. It is the standard. Otherwise patches won’t apply. There is no point in sending patches that other developers cannot apply. Use Git. >> > > 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?
Please learn how to use git.
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’t work either.
I personally won’t 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.
Please learn how to use git.
All said about this above.
>>> >>>>> This generated a short diff without real corrections only. >>>>> >>>>>>> + if($key == $szc) { #add >>>>>>> + @current2[$key] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>> + # sort newly added/modified entry >>>>>>> + &sortcurrent2; >>>>>> >>>>>> Are you sure we can sort here? >>>>>> >>>>>> See: https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=31672dc8bdb223ebf425... >>>>>> >>>>> >>>>> Yes! Why not? >>>> >>>> Because of the commit I referred to. >>> >>> Didn't understand this commit, because of lack of commentation. ;) >> >> And you didn’t ask any questions then? >> > > 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 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.
I did not test this, but I could imagine that this could happen here, too.
I know from reading and understandig the code, that this works. The approval by Matthias Fischer shows, that I'm right. See the related forum thread. Software development and code review is no field of imagination.
>>>> >>>>> >>>>>>> + &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>> + $dhcpsettings{'KEY2'} = ''; >>>>>>> + } else { #edit >>>>>>> unshift (@current2, "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"); >>>>>>> &General::log($Lang::tr{'fixed ip lease added'}); >>>>>>> >>>>>>> # Enter edit mode >>>>>>> $dhcpsettings{'KEY2'} = 0; >>>>>>> + } >>>>>>> } else { >>>>>>> @current2[$dhcpsettings{'KEY2'}] = "$dhcpsettings{'FIX_MAC'},$dhcpsettings{'FIX_ADDR'},$dhcpsettings{'FIX_ENABLED'},$dhcpsettings{'FIX_NEXTADDR'},$dhcpsettings{'FIX_FILENAME'},$dhcpsettings{'FIX_ROOTPATH'},$dhcpsettings{'FIX_REMARK'}\n"; >>>>>>> $dhcpsettings{'KEY2'} = ''; # End edit mode >>>>>>> -- >>>>>>> 2.18.0 >>>>>>> >>>>>> >>>>>> -Michael >>>>>> >>>>>> >>>>> >>>>> Again, my intention was a quick resolution for the behaviour mentioned in the forum post, which initiated my code review of dhcpi.cgi >>>> >>>> Okay, I will try to explain this one last time - not just only for you, but generally: >>>> >>>> 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. >>> >>> 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. >>> I agree fully with your aim, but this means readability also! This means some ( many? ) 'cosmetic' changes in the future, IMHO. Is this accepted? >> >> 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 problems than it solves? >> >> So no, it is not accepted. >> > > Where are the problems? > Be more specific please.
Read my first email on the patch. That is as specific as it gets.
I'll do this. But I don't know whether this changes the my opinion about the fixed problem.
>>>> 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 tested. It is all in the link that I sent you. >>>> >>>> This is not a competition about who can submit patches the fastest. >>>> >>> >>> 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 greater 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 bugzilla topic by Matthias. >>> >>>>> I found a couple of behaviours of this page, which are not obvious or straight forward ( adding dynamic leases to static leases, for example, maybe this is invoked now by my modification). >>>> >>>> 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 be fixed or of things won’t just become worse. >>>> >>> >>> 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. >> >> 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. >> > > Helping users with known problems, which could be resolved, is wating time also. 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 often is not enough. Something that works for one person does not work for another. We can never break backwards-compatibility. >> >> 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. >> > > 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.
see above.
>> 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 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 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. >> > > 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.
There is no need to behave like this. I'll be quiet myself upon source quality and proposed fixes for bugs.
>>>>> 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. >>>> >>>> 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. >>>> >>> >>> Agreed, too. But again, this special problem appears from time to time in the forums. >> >> Why do we not have a ticket on BZ then? >> > > The ticket is there ( see subject of this discussion ) and the solution exists.
I disagree with this “solution”.
Why?
-Bernhard
> > -Bernhard > >>> It is no problem, to describe the behaviour each time ( define fixed lease -> 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 canonical development process, IMHO. The patch can be applied by any core developper reading DevList/bugzilla/forums regulary. >> >> It is entirely up to you how you develop your code. How it is submitted to the list is clear. >> >> -Michael >> >>> >>> -Bernhard >>> >>>> -Michael >>>> >>>>> >>>>> Regards, >>>>> Bernhard