Re: [PATCH v2 1/6] zoneconf.cgi: Change NIC display order, improve code

Re: [PATCH v2 1/6] zoneconf.cgi: Change NIC display order, improve code

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]

Hello,

> On 18 Feb 2021, at 14:36, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Hi all,
> 
> you probably already saw this patchset last month :)
> 
> I have implemented your suggestions and decided to resubmit the entire patchset to keep it coherent.
> The patches 1-4 are unchanged. Daniel and Michael have already tested them (thank you very much!).
> 
> Patch 5 makes zoneconf use the new zone detection functions in network-functions.pl. This also fixes bug #12568!
> Patch 6 adds default values to the input fields. I decided to add a default VLAN ID as well, because these inputs suffered from the same usability issue as the STP priority fields.
> 
> I'm looking forward to your feedback!

Thank you very much for working on this.

Judging from the code, this looks very good. Nice and tidy and changes are split into small chunks that are very easy to review.

I will wait for some more people to give any feedback - and hopefully test it live - and merge it.

Well done!

-Michael

> Best regards,
> Leo
> 
> P.S. @Daniel thanks for your help with re-submitting! I have read through all your emails again and decided that I can submit these patches now.
> 

Re: [PATCH v2 1/6] zoneconf.cgi: Change NIC display order, improve code

[Leo Hofmann]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

Hello Michael,

thank you for all your positive feedback and support! I like the working atmosphere here very much.
(Sorry for the "off-topic" mail, but I think this is worth mentioning and appreciating!)

Looking forward to the test results!

Best,
Leo

Am 19.02.2021 um 20:26 schrieb Michael Tremer:
> Hello,
>
>> On 18 Feb 2021, at 14:36, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
>>
>> Hi all,
>>
>> you probably already saw this patchset last month :)
>>
>> I have implemented your suggestions and decided to resubmit the entire patchset to keep it coherent.
>> The patches 1-4 are unchanged. Daniel and Michael have already tested them (thank you very much!).
>>
>> Patch 5 makes zoneconf use the new zone detection functions in network-functions.pl. This also fixes bug #12568!
>> Patch 6 adds default values to the input fields. I decided to add a default VLAN ID as well, because these inputs suffered from the same usability issue as the STP priority fields.
>>
>> I'm looking forward to your feedback!
> Thank you very much for working on this.
>
> Judging from the code, this looks very good. Nice and tidy and changes are split into small chunks that are very easy to review.
>
> I will wait for some more people to give any feedback - and hopefully test it live - and merge it.
>
> Well done!
>
> -Michael
>
>> Best regards,
>> Leo
>>
>> P.S. @Daniel thanks for your help with re-submitting! I have read through all your emails again and decided that I can submit these patches now.
>>

Re: [PATCH v2 1/6] zoneconf.cgi: Change NIC display order, improve code

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

*thumbs up*

Couldn’t agree more.

> On 21 Feb 2021, at 12:14, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Hello Michael,
> 
> thank you for all your positive feedback and support! I like the working atmosphere here very much.
> (Sorry for the "off-topic" mail, but I think this is worth mentioning and appreciating!)
> 
> Looking forward to the test results!
> 
> Best,
> Leo
> 
> Am 19.02.2021 um 20:26 schrieb Michael Tremer:
>> Hello,
>> 
>>> On 18 Feb 2021, at 14:36, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
>>> 
>>> Hi all,
>>> 
>>> you probably already saw this patchset last month :)
>>> 
>>> I have implemented your suggestions and decided to resubmit the entire patchset to keep it coherent.
>>> The patches 1-4 are unchanged. Daniel and Michael have already tested them (thank you very much!).
>>> 
>>> Patch 5 makes zoneconf use the new zone detection functions in network-functions.pl. This also fixes bug #12568!
>>> Patch 6 adds default values to the input fields. I decided to add a default VLAN ID as well, because these inputs suffered from the same usability issue as the STP priority fields.
>>> 
>>> I'm looking forward to your feedback!
>> Thank you very much for working on this.
>> 
>> Judging from the code, this looks very good. Nice and tidy and changes are split into small chunks that are very easy to review.
>> 
>> I will wait for some more people to give any feedback - and hopefully test it live - and merge it.
>> 
>> Well done!
>> 
>> -Michael
>> 
>>> Best regards,
>>> Leo
>>> 
>>> P.S. @Daniel thanks for your help with re-submitting! I have read through all your emails again and decided that I can submit these patches now.
>>> 

[PATCH v2 1/6] zoneconf.cgi: Change NIC display order, improve code

[Leo-Andres Hofmann]

[-- Attachment #1: Type: text/plain, Size: 4763 bytes --]

Refactor duplicate perl code and add comments

Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
---
 html/cgi-bin/zoneconf.cgi | 53 +++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index 0914ceb78..bf46ab0c7 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -26,6 +26,7 @@ require '/var/ipfire/general-functions.pl';
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
 
+###--- HTML HEAD ---###
 my $extraHead = <<END
 <style>
 	table#zoneconf {
@@ -105,7 +106,9 @@ my $extraHead = <<END
 <script src="/include/zoneconf.js"></script>
 END
 ;
+###--- END HTML HEAD ---###
 
+### Read configuration ###
 my %ethsettings = ();
 my %vlansettings = ();
 my %cgiparams = ();
@@ -119,7 +122,7 @@ my $restart_notice = "";
 &Header::showhttpheaders();
 
 # Define all zones we will check for NIC assignment
-my @zones = ("green", "red", "orange", "blue");
+my @zones = ("red", "green", "orange", "blue");
 
 # Get all physical NICs present
 opendir(my $dh, "/sys/class/net/");
@@ -153,6 +156,21 @@ foreach (@nics) {
 	}
 }
 
+### Functions ###
+
+# Check if a zone is in IP mode or in PPP, PPPoE, VDSL, ... mode
+sub is_zonetype_ip {
+	my $zone_type = shift;
+	return ($zone_type eq "STATIC" || $zone_type eq "DHCP");
+}
+
+# Check if a zone is activated (device assigned)
+sub is_zone_activated {
+	my $zone = uc shift;
+	return ($ethsettings{"${zone}_DEV"} ne "");
+}
+
+### START PAGE ###
 &Header::openpage($Lang::tr{"zoneconf title"}, 1, $extraHead);
 &Header::openbigbox('100%', 'center');
 
@@ -195,6 +213,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
 				}
 			}
 
+			# skip NIC/VLAN assignment and additional zone options for RED in PPP mode
 			next;
 		}
 
@@ -278,6 +297,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
 		}
 	}
 
+	# validation failed, show error message and exit
 	if ($VALIDATE_error) {
 		&Header::openbox('100%', 'left', $Lang::tr{"error"});
 
@@ -290,16 +310,17 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
 		exit 0;
 	}
 
+	# new settings are valid, write configuration files
 	&General::writehash("${General::swroot}/ethernet/settings",\%ethsettings);
 	&General::writehash("${General::swroot}/ethernet/vlans",\%vlansettings);
 
 	$restart_notice = $Lang::tr{'zoneconf notice reboot'};
 }
 
-&Header::openbox('100%', 'left', $Lang::tr{"zoneconf nic assignment"});
-
 ### START OF TABLE ###
 
+&Header::openbox('100%', 'left', $Lang::tr{"zoneconf nic assignment"});
+
 print <<END
 <form method='post' enctype='multipart/form-data'>
 	<table id="zoneconf">
@@ -311,19 +332,16 @@ END
 # Fill the table header with all activated zones
 foreach (@zones) {
 	my $uc = uc $_;
-	my $dev_name = $ethsettings{"${uc}_DEV"};
 
-	if ($dev_name eq "") { # If the zone is not activated, don't show it
-		next;
-	}
+	# If the zone is not activated, don't show it
+	next unless is_zone_activated($_);
 
-	# If the zone is in PPP mode, don't show a mode dropdown
+	# If the red zone is in PPP mode, don't show a mode dropdown
 	if ($uc eq "RED") {
 		my $red_type = $ethsettings{"RED_TYPE"};
-		my $red_restricted = ($uc eq "RED" && ! ($red_type eq "STATIC" || $red_type eq "DHCP"));
 
-		if ($red_restricted) {
-			print "\t\t<td class='heading $_'>$uc ($red_type)</td>\n";
+		unless (is_zonetype_ip($red_type)) {
+			print "\t\t<td class='heading bold $_'>$uc ($red_type)</td>\n";
 
 			next; # We're done here
 		}
@@ -354,6 +372,7 @@ END
 
 print "\t</tr>\n";
 
+# NIC assignment matrix
 foreach (@nics) {
 	my $mac = $_->[0];
 	my $nic = $_->[1];
@@ -365,19 +384,14 @@ foreach (@nics) {
 	# Iterate through all zones and check if the current NIC is assigned to it
 	foreach (@zones) {
 		my $uc = uc $_;
-		my $dev_name = $ethsettings{"${uc}_DEV"};
 		my $highlight = "";
 
-		if ($dev_name eq "") { # Again, skip the zone if it is not activated
-			next;
-		}
+		# If the zone is not activated, don't show it
+		next unless is_zone_activated($_);
 
 		if ($uc eq "RED") {
-			my $red_type = $ethsettings{"RED_TYPE"};
-			my $red_restricted = ($uc eq "RED" && ! ($red_type eq "STATIC" || $red_type eq "DHCP"));
-
 			# VLANs/Bridging is not possible if the RED interface is set to PPP, PPPoE, VDSL, ...
-			if ($red_restricted) {
+			unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
 				my $checked = "";
 
 				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
@@ -449,6 +463,7 @@ END
 	print "\t</tr>\n";
 }
 
+# footer and submit button
 print <<END
 	</table>
 
-- 
2.27.0.windows.1