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.
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.
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.
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.
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. ;)
&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?
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.
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. 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.
-Bernhard
-Michael
Regards, Bernhard