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

[PATCH 1/4] 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

[PATCH 2/4] zoneconf.cgi: Modify CSS to allow additional rows

[Leo-Andres Hofmann]

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

Simplify borders, load more colors from header and add dividers

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

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index bf46ab0c7..13543d244 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -32,39 +32,59 @@ my $extraHead = <<END
 	table#zoneconf {
 		width: 100%;
 		border-collapse: collapse;
+		border-style: hidden;
 		table-layout: fixed;
 	}
 
+	/* row height */
 	#zoneconf tr {
 		height: 4em;
 	}
+	/* section separators */
+	#zoneconf tr.divider-top {
+			border-top: 2px solid $Header::bordercolour;
+	}
+	#zoneconf tr.divider-bottom {
+			border-bottom: 2px solid $Header::bordercolour;
+	}
 
+	/* table cells */
 	#zoneconf td {
 		padding: 5px 10px;
-		border: 0.5px solid black;
+		border-left: 0.5px solid $Header::bordercolour;
 		text-align: center;
 	}
 
-	/* dark grey header cells */
+	/* grey header cells */
 	#zoneconf td.heading {
-		background-color: grey;
+		background-color: lightgrey;
 		color: white;
 	}	
-	#zoneconf td.heading::first-line {
+	#zoneconf td.heading.bold::first-line {
 		font-weight: bold;
 		line-height: 1.6;
 	}
 
-	/* narrow left column */
+	/* narrow left column with background color */
 	#zoneconf tr > td:first-child {
 		width: 11em;
 	}
+	#zoneconf tr.nic-row > td:first-child {
+			background-color: darkgray;
+	}
+	#zoneconf tr.nic-row {
+		border-bottom: 0.5px solid $Header::bordercolour;
+	}
 
 	/* alternating row background color */
+	#zoneconf tr {
+		background-color: $Header::table2colour;
+	}
 	#zoneconf tr:nth-child(2n+3) {
-		background-color: #F0F0F0;
+		background-color: $Header::table1colour;
 	}
 
+	/* special cell colors */
 	#zoneconf td.green {
 		background-color: $Header::colourgreen;
 	}
@@ -83,8 +103,6 @@ my $extraHead = <<END
 
 	#zoneconf td.topleft {
 		background-color: $Header::pagecolour;
-		border-top-style: none;
-		border-left-style: none;
 	}
 
 	input.vlanid {
@@ -324,7 +342,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
 print <<END
 <form method='post' enctype='multipart/form-data'>
 	<table id="zoneconf">
-	<tr>
+	<tr class="divider-bottom">
 		<td class="topleft"></td>
 END
 ;
@@ -359,7 +377,7 @@ foreach (@zones) {
 	}
 
 	print <<END
-		<td class='heading $_'>$uc<br>
+		<td class='heading bold $_'>$uc<br>
 			<select name="MODE $uc">
 				<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf nicmode default"}</option>
 				<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf nicmode bridge"}</option>
@@ -378,8 +396,8 @@ foreach (@nics) {
 	my $nic = $_->[1];
 	my $wlan = $_->[2];
 
-	print "\t<tr>\n";
-	print "\t\t<td class='heading'>$nic<br>$mac</td>\n";
+	print "\t<tr class='nic-row'>\n";
+	print "\t\t<td class='heading bold'>$nic<br>$mac</td>\n";
 
 	# Iterate through all zones and check if the current NIC is assigned to it
 	foreach (@zones) {
-- 
2.27.0.windows.1

[PATCH 3/4] zoneconf.cgi: Add STP options to GUI

[Leo-Andres Hofmann]

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

Changes & new features:
- Add CSS for STP options, add texts to language files
- Read STP settings from ethernet configuration and display inputs
- Validate and save STP settings

Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
---
 html/cgi-bin/zoneconf.cgi | 100 ++++++++++++++++++++++++++++++++++++++
 langs/de/cgi-bin/de.pl    |   4 ++
 langs/en/cgi-bin/en.pl    |   4 ++
 3 files changed, 108 insertions(+)

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index 13543d244..1d30450ed 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -40,6 +40,13 @@ my $extraHead = <<END
 	#zoneconf tr {
 		height: 4em;
 	}
+	#zoneconf tr.half-height {
+		height: 2em;
+	}
+	#zoneconf tr.half-height > td {
+		padding: 2px 10px;
+	}
+
 	/* section separators */
 	#zoneconf tr.divider-top {
 			border-top: 2px solid $Header::bordercolour;
@@ -75,6 +82,9 @@ my $extraHead = <<END
 	#zoneconf tr.nic-row {
 		border-bottom: 0.5px solid $Header::bordercolour;
 	}
+	#zoneconf tr.option-row > td:first-child {
+			background-color: gray;
+	}
 
 	/* alternating row background color */
 	#zoneconf tr {
@@ -108,6 +118,9 @@ my $extraHead = <<END
 	input.vlanid {
 		width: 4em;
 	}
+	input.stp-priority {
+		width: 5em;
+	}
 
 	#submit-container {
 		width: 100%;
@@ -313,6 +326,25 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
 		} elsif ($zone_mode eq "MACVTAP") {
 			$ethsettings{"${uc}_MODE"} = "macvtap";
 		}
+
+		# STP options
+		# (this has already been skipped when RED is in PPP mode, so we don't need to check for PPP here)
+		$ethsettings{"${uc}_STP"} = "";
+		my $stp_enabled = $cgiparams{"STP-$uc"} eq "on";
+		my $stp_priority = $cgiparams{"STP-PRIORITY-$uc"};
+
+		if($stp_enabled) {
+			unless($ethsettings{"${uc}_MODE"} eq "bridge") { # STP is only available in bridge mode
+				$VALIDATE_error = $Lang::tr{"zoneconf val stp zone mode error"};
+				last;
+			}
+			unless (looks_like_number($stp_priority) && ($stp_priority >= 1) && ($stp_priority <= 65535)) { # STP bridge priority range: 1..65535
+				$VALIDATE_error = $Lang::tr{"zoneconf val stp priority range error"};
+				last;
+			}
+			$ethsettings{"${uc}_STP"} = "on"; # network-hotplug-bridges expects "on"
+			$ethsettings{"${uc}_STP_PRIORITY"} = $stp_priority;
+		}
 	}
 
 	# validation failed, show error message and exit
@@ -481,6 +513,74 @@ END
 	print "\t</tr>\n";
 }
 
+# STP options
+my @stp_html = (); # form fields buffer (two rows)
+
+foreach (@zones) { # load settings and prepare form elements for each zone
+	my $uc = uc $_;
+
+	# skip if zone is not activated
+	next unless is_zone_activated($_);
+
+	# STP is not available if the RED interface is set to PPP, PPPoE, VDSL, ...
+	if ($uc eq "RED") {
+		unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
+			push(@stp_html, ["\t\t<td></td>\n", "\t\t<td></td>\n"]); # print empty cell
+			next;
+		}
+	}
+
+	# load configuration
+	my $stp_available = $ethsettings{"${uc}_MODE"} eq "bridge"; # STP is only available in bridge mode
+	my $stp_enabled = $ethsettings{"${uc}_STP"} eq "on";
+	my $stp_priority = $ethsettings{"${uc}_STP_PRIORITY"};
+
+	# form element modifiers
+	my $checked = "";
+	my $disabled = "";
+	$checked = "checked" if ($stp_available && $stp_enabled);
+	$disabled = "disabled" unless $stp_available;
+
+	# enable checkbox HTML
+	my $row_1 = <<END
+		<td>
+			<input type="checkbox" name="STP-$uc" $disabled $checked>
+		</td>
+END
+;
+	$disabled = "disabled" unless $stp_enabled; # STP priority can't be entered if STP is disabled
+
+	# priority input box HTML
+	my $row_2 = <<END
+		<td>
+			<input type="number" class="stp-priority" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
+		</td>
+END
+;
+	# add fields to buffer
+	push(@stp_html, [$row_1, $row_2]);
+}
+
+# print two rows of prepared form elements
+print <<END
+	<tr class="half-height divider-top option-row">
+		<td class="heading bold">$Lang::tr{"zoneconf stp enable"}</td>
+END
+;
+foreach (@stp_html) {
+	print $_->[0]; # row 1
+}
+print <<END
+	</tr>
+	<tr class="half-height option-row">
+		<td class="heading">$Lang::tr{"zoneconf stp priority"}</td>
+END
+;
+foreach (@stp_html) {
+	print $_->[1]; # row 2
+}
+print "\t</tr>\n";
+
 # footer and submit button
 print <<END
 	</table>
diff --git a/langs/de/cgi-bin/de.pl b/langs/de/cgi-bin/de.pl
index 38c9783f8..f3e4aca7e 100644
--- a/langs/de/cgi-bin/de.pl
+++ b/langs/de/cgi-bin/de.pl
@@ -2977,9 +2977,13 @@
 'zoneconf nicmode default' => 'Normal',
 'zoneconf nicmode macvtap' => 'MacVTap',
 'zoneconf notice reboot' => 'Bitte einen Neustart durchführen, um die Änderungen zu übernehmen.',
+'zoneconf stp enable' => 'STP aktivieren',
+'zoneconf stp priority' => 'Brücke Priorität',
 'zoneconf title' => 'Zonen einrichten',
 'zoneconf val native assignment error' => 'Eine Netzwerkkarte kann nicht von mehreren Zonen nativ verwendet werden.',
 'zoneconf val ppp assignment error' => 'Die Netzwerkkarte, die von RED im PPP-Modus verwendet wird, kann keiner anderen Zone zugeordnet werden.',
+'zoneconf val stp priority range error' => 'STP Brücke Priorität muss im Bereich 1-65535 liegen',
+'zoneconf val stp zone mode error' => 'STP kann nur aktiviert werden, wenn sich die Zone im Brückenmodus befindet',
 'zoneconf val vlan amount assignment error' => 'Pro Zone kann nur ein VLAN verwendet werden.',
 'zoneconf val vlan tag assignment error' => 'Pro Netzwerkkarte kann derselbe VLAN-Tag nur einmal verwendet werden.',
 'zoneconf val zoneslave amount error' => 'Wenn eine Zone nicht im Brückenmodus ist, kann ihr nur eine Netzwerkkarte zugewiesen werden.',
diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl
index 500913240..649139c73 100644
--- a/langs/en/cgi-bin/en.pl
+++ b/langs/en/cgi-bin/en.pl
@@ -3025,9 +3025,13 @@
 'zoneconf nicmode default' => 'Default',
 'zoneconf nicmode macvtap' => 'MacVTap',
 'zoneconf notice reboot' => 'Please reboot to apply your changes.',
+'zoneconf stp enable' => 'STP enable',
+'zoneconf stp priority' => 'Bridge Priority',
 'zoneconf title' => 'Zone Configuration',
 'zoneconf val native assignment error' => 'A NIC cannot be accessed natively by more than one zone.',
 'zoneconf val ppp assignment error' => 'The NIC used for RED in PPP mode cannot be accessed by any other zone.',
+'zoneconf val stp priority range error' => 'STP bridge priority must be in the range of 1-65535',
+'zoneconf val stp zone mode error' => 'STP can only be enabled if the zone is in bridge mode',
 'zoneconf val vlan amount assignment error' => 'A zone cannot have more than one VLAN assigned.',
 'zoneconf val vlan tag assignment error' => 'You cannot use the same VLAN tag more than once per NIC.',
 'zoneconf val zoneslave amount error' => 'A zone that is not in bridge mode can\'t have more than one NIC assigned',
-- 
2.27.0.windows.1

[PATCH 4/4] zoneconf.cgi: Add Javascript for new GUI elements

[Leo-Andres Hofmann]

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

Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
---
 html/cgi-bin/zoneconf.cgi     |  6 +++---
 html/html/include/zoneconf.js | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index 1d30450ed..eb6cd0e66 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -410,7 +410,7 @@ foreach (@zones) {
 
 	print <<END
 		<td class='heading bold $_'>$uc<br>
-			<select name="MODE $uc">
+			<select name="MODE $uc" data-zone="$uc" onchange="changeZoneMode(this)">
 				<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf nicmode default"}</option>
 				<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf nicmode bridge"}</option>
 				<option value="MACVTAP" $mode_selected{"MACVTAP"}>$Lang::tr{"zoneconf nicmode macvtap"}</option>
@@ -544,7 +544,7 @@ foreach (@zones) { # load settings and prepare form elements for each zone
 	# enable checkbox HTML
 	my $row_1 = <<END
 		<td>
-			<input type="checkbox" name="STP-$uc" $disabled $checked>
+			<input type="checkbox" id="STP-$uc" name="STP-$uc" data-zone="$uc" onchange="changeEnableSTP(this)" $disabled $checked>
 		</td>
 END
 ;
@@ -553,7 +553,7 @@ END
 	# priority input box HTML
 	my $row_2 = <<END
 		<td>
-			<input type="number" class="stp-priority" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
+			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
 		</td>
 END
 ;
diff --git a/html/html/include/zoneconf.js b/html/html/include/zoneconf.js
index d76f0ab68..d27a79bc8 100644
--- a/html/html/include/zoneconf.js
+++ b/html/html/include/zoneconf.js
@@ -54,3 +54,30 @@ function highlightAccess(selectObj) {
 	//if interface is assigned, highlight table cell in zone color
 	colorParentCell(selectObj, zoneColor, (selectObj.value !== 'NONE'));
 }
+
+//update zone mode
+function changeZoneMode(selectObj) {
+	if(!(selectObj && ('zone' in selectObj.dataset))) {
+		return; //required parameters are missing
+	}
+
+	// STP enable checkbox
+	let stpEnable = document.getElementById('STP-' + selectObj.dataset.zone);
+	if(stpEnable) {
+		stpEnable.disabled = (selectObj.value !== 'BRIDGE'); //STP is available if zone is in bridge mode
+		stpEnable.checked = stpEnable.checked && (! stpEnable.disabled); //un-check if disabled
+		stpEnable.dispatchEvent(new Event('change'));
+	}
+}
+
+//STP enable checkbox change toggles priority input
+function changeEnableSTP(inputObj) {
+	if(!(inputObj && ('zone' in inputObj.dataset))) {
+		return; //required parameters are missing
+	}
+
+	let priority = document.getElementById('STP-PRIORITY-' + inputObj.dataset.zone);
+	if(priority) {
+		priority.disabled = inputObj.disabled || (! inputObj.checked);
+	}
+}
-- 
2.27.0.windows.1

Re: [PATCH 4/4] zoneconf.cgi: Add Javascript for new GUI elements

[Leo Hofmann]

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

Hi Daniel,

just in time for the holidays, I finished my work on the zoneconf.cgi STP GUI.
I'm not sure how to test STP in my development setup, but I think I got it right. I would appreciate if you could test these patches, thank you :)

Best regards & happy holidays
Leo

Am 22.12.2020 um 21:06 schrieb Leo-Andres Hofmann:
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
>   html/cgi-bin/zoneconf.cgi     |  6 +++---
>   html/html/include/zoneconf.js | 27 +++++++++++++++++++++++++++
>   2 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
> index 1d30450ed..eb6cd0e66 100644
> --- a/html/cgi-bin/zoneconf.cgi
> +++ b/html/cgi-bin/zoneconf.cgi
> @@ -410,7 +410,7 @@ foreach (@zones) {
>   
>   	print <<END
>   		<td class='heading bold $_'>$uc<br>
> -			<select name="MODE $uc">
> +			<select name="MODE $uc" data-zone="$uc" onchange="changeZoneMode(this)">
>   				<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf nicmode default"}</option>
>   				<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf nicmode bridge"}</option>
>   				<option value="MACVTAP" $mode_selected{"MACVTAP"}>$Lang::tr{"zoneconf nicmode macvtap"}</option>
> @@ -544,7 +544,7 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>   	# enable checkbox HTML
>   	my $row_1 = <<END
>   		<td>
> -			<input type="checkbox" name="STP-$uc" $disabled $checked>
> +			<input type="checkbox" id="STP-$uc" name="STP-$uc" data-zone="$uc" onchange="changeEnableSTP(this)" $disabled $checked>
>   		</td>
>   END
>   ;
> @@ -553,7 +553,7 @@ END
>   	# priority input box HTML
>   	my $row_2 = <<END
>   		<td>
> -			<input type="number" class="stp-priority" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
> +			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
>   		</td>
>   END
>   ;
> diff --git a/html/html/include/zoneconf.js b/html/html/include/zoneconf.js
> index d76f0ab68..d27a79bc8 100644
> --- a/html/html/include/zoneconf.js
> +++ b/html/html/include/zoneconf.js
> @@ -54,3 +54,30 @@ function highlightAccess(selectObj) {
>   	//if interface is assigned, highlight table cell in zone color
>   	colorParentCell(selectObj, zoneColor, (selectObj.value !== 'NONE'));
>   }
> +
> +//update zone mode
> +function changeZoneMode(selectObj) {
> +	if(!(selectObj && ('zone' in selectObj.dataset))) {
> +		return; //required parameters are missing
> +	}
> +
> +	// STP enable checkbox
> +	let stpEnable = document.getElementById('STP-' + selectObj.dataset.zone);
> +	if(stpEnable) {
> +		stpEnable.disabled = (selectObj.value !== 'BRIDGE'); //STP is available if zone is in bridge mode
> +		stpEnable.checked = stpEnable.checked && (! stpEnable.disabled); //un-check if disabled
> +		stpEnable.dispatchEvent(new Event('change'));
> +	}
> +}
> +
> +//STP enable checkbox change toggles priority input
> +function changeEnableSTP(inputObj) {
> +	if(!(inputObj && ('zone' in inputObj.dataset))) {
> +		return; //required parameters are missing
> +	}
> +
> +	let priority = document.getElementById('STP-PRIORITY-' + inputObj.dataset.zone);
> +	if(priority) {
> +		priority.disabled = inputObj.disabled || (! inputObj.checked);
> +	}
> +}

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

[Michael Tremer]

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

Hi,

> On 22 Dec 2020, at 21:06, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> 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 "");
> +}

Should we not have functions like this in network-functions.pl, so that we do not have to rewrite them every time?

We also have functions for this in general-functions.pl which use the CONFIG_TYPE setting from /var/ipfire/ethernet/settings. That should actually be the correct way to do this.

Maybe this is one for the list of things to tidy up :)

> +
> +### 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;
> 		}

I like well-commented code.

> @@ -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($_);

And this is a good change, because it clearly says what the code is supposed to do.

> 		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
> 

Re: [PATCH 4/4] zoneconf.cgi: Add Javascript for new GUI elements

[Michael Tremer]

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

Hi,

This patchset looks good to me, but I would like to hear back from Daniel for some test results :)

Breaking people’s networks would be really bad. So let’s give this a good test.

-Michael

> On 22 Dec 2020, at 21:15, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Hi Daniel,
> 
> just in time for the holidays, I finished my work on the zoneconf.cgi STP GUI.
> I'm not sure how to test STP in my development setup, but I think I got it right. I would appreciate if you could test these patches, thank you :)
> 
> Best regards & happy holidays
> Leo
> 
> Am 22.12.2020 um 21:06 schrieb Leo-Andres Hofmann:
>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>> ---
>>  html/cgi-bin/zoneconf.cgi     |  6 +++---
>>  html/html/include/zoneconf.js | 27 +++++++++++++++++++++++++++
>>  2 files changed, 30 insertions(+), 3 deletions(-)
>> 
>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>> index 1d30450ed..eb6cd0e66 100644
>> --- a/html/cgi-bin/zoneconf.cgi
>> +++ b/html/cgi-bin/zoneconf.cgi
>> @@ -410,7 +410,7 @@ foreach (@zones) {
>>    	print <<END
>>  		<td class='heading bold $_'>$uc<br>
>> -			<select name="MODE $uc">
>> +			<select name="MODE $uc" data-zone="$uc" onchange="changeZoneMode(this)">
>>  				<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf nicmode default"}</option>
>>  				<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf nicmode bridge"}</option>
>>  				<option value="MACVTAP" $mode_selected{"MACVTAP"}>$Lang::tr{"zoneconf nicmode macvtap"}</option>
>> @@ -544,7 +544,7 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>>  	# enable checkbox HTML
>>  	my $row_1 = <<END
>>  		<td>
>> -			<input type="checkbox" name="STP-$uc" $disabled $checked>
>> +			<input type="checkbox" id="STP-$uc" name="STP-$uc" data-zone="$uc" onchange="changeEnableSTP(this)" $disabled $checked>
>>  		</td>
>>  END
>>  ;
>> @@ -553,7 +553,7 @@ END
>>  	# priority input box HTML
>>  	my $row_2 = <<END
>>  		<td>
>> -			<input type="number" class="stp-priority" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
>> +			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
>>  		</td>
>>  END
>>  ;
>> diff --git a/html/html/include/zoneconf.js b/html/html/include/zoneconf.js
>> index d76f0ab68..d27a79bc8 100644
>> --- a/html/html/include/zoneconf.js
>> +++ b/html/html/include/zoneconf.js
>> @@ -54,3 +54,30 @@ function highlightAccess(selectObj) {
>>  	//if interface is assigned, highlight table cell in zone color
>>  	colorParentCell(selectObj, zoneColor, (selectObj.value !== 'NONE'));
>>  }
>> +
>> +//update zone mode
>> +function changeZoneMode(selectObj) {
>> +	if(!(selectObj && ('zone' in selectObj.dataset))) {
>> +		return; //required parameters are missing
>> +	}
>> +
>> +	// STP enable checkbox
>> +	let stpEnable = document.getElementById('STP-' + selectObj.dataset.zone);
>> +	if(stpEnable) {
>> +		stpEnable.disabled = (selectObj.value !== 'BRIDGE'); //STP is available if zone is in bridge mode
>> +		stpEnable.checked = stpEnable.checked && (! stpEnable.disabled); //un-check if disabled
>> +		stpEnable.dispatchEvent(new Event('change'));
>> +	}
>> +}
>> +
>> +//STP enable checkbox change toggles priority input
>> +function changeEnableSTP(inputObj) {
>> +	if(!(inputObj && ('zone' in inputObj.dataset))) {
>> +		return; //required parameters are missing
>> +	}
>> +
>> +	let priority = document.getElementById('STP-PRIORITY-' + inputObj.dataset.zone);
>> +	if(priority) {
>> +		priority.disabled = inputObj.disabled || (! inputObj.checked);
>> +	}
>> +}

Re: [PATCH 4/4] zoneconf.cgi: Add Javascript for new GUI elements

[Leo Hofmann]

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

Hi Michael,

thank you very much for reviewing my patches! I agree, we should test this thoroughly.
I would rather submit new patches than break someones network :)

Regarding your other mail, I looked into moving the "is_zone_active"... functions to the general-functions.pl file.
I still need to make sure that these functions are universally applicable, then I will submit them.

Regards,
Leo

Am 27.12.2020 um 12:05 schrieb Michael Tremer:
> Hi,
>
> This patchset looks good to me, but I would like to hear back from Daniel for some test results :)
>
> Breaking people’s networks would be really bad. So let’s give this a good test.
>
> -Michael
>
>> On 22 Dec 2020, at 21:15, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
>>
>> Hi Daniel,
>>
>> just in time for the holidays, I finished my work on the zoneconf.cgi STP GUI.
>> I'm not sure how to test STP in my development setup, but I think I got it right. I would appreciate if you could test these patches, thank you :)
>>
>> Best regards & happy holidays
>> Leo
>>
>> Am 22.12.2020 um 21:06 schrieb Leo-Andres Hofmann:
>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>> ---
>>>   html/cgi-bin/zoneconf.cgi     |  6 +++---
>>>   html/html/include/zoneconf.js | 27 +++++++++++++++++++++++++++
>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>>> index 1d30450ed..eb6cd0e66 100644
>>> --- a/html/cgi-bin/zoneconf.cgi
>>> +++ b/html/cgi-bin/zoneconf.cgi
>>> @@ -410,7 +410,7 @@ foreach (@zones) {
>>>     	print <<END
>>>   		<td class='heading bold $_'>$uc<br>
>>> -			<select name="MODE $uc">
>>> +			<select name="MODE $uc" data-zone="$uc" onchange="changeZoneMode(this)">
>>>   				<option value="DEFAULT" $mode_selected{"DEFAULT"}>$Lang::tr{"zoneconf nicmode default"}</option>
>>>   				<option value="BRIDGE" $mode_selected{"BRIDGE"}>$Lang::tr{"zoneconf nicmode bridge"}</option>
>>>   				<option value="MACVTAP" $mode_selected{"MACVTAP"}>$Lang::tr{"zoneconf nicmode macvtap"}</option>
>>> @@ -544,7 +544,7 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>>>   	# enable checkbox HTML
>>>   	my $row_1 = <<END
>>>   		<td>
>>> -			<input type="checkbox" name="STP-$uc" $disabled $checked>
>>> +			<input type="checkbox" id="STP-$uc" name="STP-$uc" data-zone="$uc" onchange="changeEnableSTP(this)" $disabled $checked>
>>>   		</td>
>>>   END
>>>   ;
>>> @@ -553,7 +553,7 @@ END
>>>   	# priority input box HTML
>>>   	my $row_2 = <<END
>>>   		<td>
>>> -			<input type="number" class="stp-priority" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
>>> +			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535" value="$stp_priority" $disabled>
>>>   		</td>
>>>   END
>>>   ;
>>> diff --git a/html/html/include/zoneconf.js b/html/html/include/zoneconf.js
>>> index d76f0ab68..d27a79bc8 100644
>>> --- a/html/html/include/zoneconf.js
>>> +++ b/html/html/include/zoneconf.js
>>> @@ -54,3 +54,30 @@ function highlightAccess(selectObj) {
>>>   	//if interface is assigned, highlight table cell in zone color
>>>   	colorParentCell(selectObj, zoneColor, (selectObj.value !== 'NONE'));
>>>   }
>>> +
>>> +//update zone mode
>>> +function changeZoneMode(selectObj) {
>>> +	if(!(selectObj && ('zone' in selectObj.dataset))) {
>>> +		return; //required parameters are missing
>>> +	}
>>> +
>>> +	// STP enable checkbox
>>> +	let stpEnable = document.getElementById('STP-' + selectObj.dataset.zone);
>>> +	if(stpEnable) {
>>> +		stpEnable.disabled = (selectObj.value !== 'BRIDGE'); //STP is available if zone is in bridge mode
>>> +		stpEnable.checked = stpEnable.checked && (! stpEnable.disabled); //un-check if disabled
>>> +		stpEnable.dispatchEvent(new Event('change'));
>>> +	}
>>> +}
>>> +
>>> +//STP enable checkbox change toggles priority input
>>> +function changeEnableSTP(inputObj) {
>>> +	if(!(inputObj && ('zone' in inputObj.dataset))) {
>>> +		return; //required parameters are missing
>>> +	}
>>> +
>>> +	let priority = document.getElementById('STP-PRIORITY-' + inputObj.dataset.zone);
>>> +	if(priority) {
>>> +		priority.disabled = inputObj.disabled || (! inputObj.checked);
>>> +	}
>>> +}