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

[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

[PATCH v2 2/6] 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 v2 3/6] 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 307b8a97c..b3c2a69da 100644
--- a/langs/de/cgi-bin/de.pl
+++ b/langs/de/cgi-bin/de.pl
@@ -2979,9 +2979,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 22e8a4cc6..10167f872 100644
--- a/langs/en/cgi-bin/en.pl
+++ b/langs/en/cgi-bin/en.pl
@@ -3027,9 +3027,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 v2 4/6] 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

[PATCH v2 5/6] zoneconf.cgi: Import network-functions.pl

[Leo-Andres Hofmann]

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

Remove custom functions and use network-functions.pl instead to detect
the available zones correctly. This also removes the requirement that
a device must be assigned for a zone to become visible/configurable.

Fixes: #12568

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

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index eb6cd0e66..9d01d06ce 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -25,6 +25,7 @@ use Scalar::Util qw(looks_like_number);
 require '/var/ipfire/general-functions.pl';
 require "${General::swroot}/lang.pl";
 require "${General::swroot}/header.pl";
+require "${General::swroot}/network-functions.pl";
 
 ###--- HTML HEAD ---###
 my $extraHead = <<END
@@ -152,8 +153,8 @@ my $restart_notice = "";
 &Header::getcgihash(\%cgiparams);
 &Header::showhttpheaders();
 
-# Define all zones we will check for NIC assignment
-my @zones = ("red", "green", "orange", "blue");
+# Get all network zones that are currently enabled
+my @zones = Network::get_available_network_zones();
 
 # Get all physical NICs present
 opendir(my $dh, "/sys/class/net/");
@@ -187,20 +188,6 @@ 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');
@@ -211,7 +198,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
 	my %VALIDATE_nic_check = ();
 	my $VALIDATE_error = "";
 
-	foreach (@zones) {
+	# Loop trough all known zones to ensure a complete configuration file is created
+	foreach (@Network::known_network_zones) {
 		my $uc = uc $_;
 		my $slave_string = "";
 		my $zone_mode = $cgiparams{"MODE $uc"};
@@ -383,14 +371,11 @@ END
 foreach (@zones) {
 	my $uc = uc $_;
 
-	# If the zone is not activated, don't show it
-	next unless is_zone_activated($_);
-
 	# If the red zone is in PPP mode, don't show a mode dropdown
 	if ($uc eq "RED") {
 		my $red_type = $ethsettings{"RED_TYPE"};
 
-		unless (is_zonetype_ip($red_type)) {
+		unless (Network::is_red_mode_ip()) {
 			print "\t\t<td class='heading bold $_'>$uc ($red_type)</td>\n";
 
 			next; # We're done here
@@ -436,12 +421,9 @@ foreach (@nics) {
 		my $uc = uc $_;
 		my $highlight = "";
 
-		# If the zone is not activated, don't show it
-		next unless is_zone_activated($_);
-
 		if ($uc eq "RED") {
 			# VLANs/Bridging is not possible if the RED interface is set to PPP, PPPoE, VDSL, ...
-			unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
+			unless (Network::is_red_mode_ip()) {
 				my $checked = "";
 
 				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
@@ -519,12 +501,9 @@ 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"})) {
+		unless (Network::is_red_mode_ip()) {
 			push(@stp_html, ["\t\t<td></td>\n", "\t\t<td></td>\n"]); # print empty cell
 			next;
 		}
-- 
2.27.0.windows.1

[PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Leo-Andres Hofmann]

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

Add default values and mark fields as required.

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

diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
index 9d01d06ce..bbd042ffc 100644
--- a/html/cgi-bin/zoneconf.cgi
+++ b/html/cgi-bin/zoneconf.cgi
@@ -478,6 +478,9 @@ END
 		if ($access_selected{"NONE"} eq "") {
 			$highlight = $_;
 		}
+		
+		# default VLAN tag if not configured
+		$zone_vlan_id = 1 unless looks_like_number($zone_vlan_id);
 
 		print <<END
 		<td class="$highlight">
@@ -486,7 +489,7 @@ END
 				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
 				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
 			</select>
-			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
+			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" required $field_disabled>
 		</td>
 END
 ;
@@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form elements for each zone
 	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"};
+	
+	# set priority to default value if no numerical value is configured
+	$stp_priority = 32768 unless looks_like_number($stp_priority);
 
 	# form element modifiers
 	my $checked = "";
@@ -532,7 +538,7 @@ END
 	# priority input box HTML
 	my $row_2 = <<END
 		<td>
-			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" 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" required $disabled>
 		</td>
 END
 ;
-- 
2.27.0.windows.1

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Michael Tremer]

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

Hello,

> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Add default values and mark fields as required.
> 
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
> index 9d01d06ce..bbd042ffc 100644
> --- a/html/cgi-bin/zoneconf.cgi
> +++ b/html/cgi-bin/zoneconf.cgi
> @@ -478,6 +478,9 @@ END
> 		if ($access_selected{"NONE"} eq "") {
> 			$highlight = $_;
> 		}
> +		
> +		# default VLAN tag if not configured
> +		$zone_vlan_id = 1 unless looks_like_number($zone_vlan_id);

I am not sure if it is a good idea to add a default here.

Isn’t there a danger that people will hit save prematurely and connect the wrong VLAN with another one?

Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t make it easier to run into that error.

> 
> 		print <<END
> 		<td class="$highlight">
> @@ -486,7 +489,7 @@ END
> 				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
> 				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
> 			</select>
> -			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
> +			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" required $field_disabled>
> 		</td>
> END
> ;
> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form elements for each zone
> 	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"};
> +	
> +	# set priority to default value if no numerical value is configured
> +	$stp_priority = 32768 unless looks_like_number($stp_priority);

This is very good.

Since this is in this patchset and comes with the dependency to the other code above, I cannot pull this in with the STP patchset where it actually belongs.

But I guess we should merge this all together anyways :)

-Michael

> 
> 	# form element modifiers
> 	my $checked = "";
> @@ -532,7 +538,7 @@ END
> 	# priority input box HTML
> 	my $row_2 = <<END
> 		<td>
> -			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" 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" required $disabled>
> 		</td>
> END
> ;
> -- 
> 2.27.0.windows.1
> 

Re: [PATCH v2 5/6] zoneconf.cgi: Import network-functions.pl

[Michael Tremer]

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



> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Remove custom functions and use network-functions.pl instead to detect
> the available zones correctly. This also removes the requirement that
> a device must be assigned for a zone to become visible/configurable.
> 
> Fixes: #12568
> 
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
> html/cgi-bin/zoneconf.cgi | 37 ++++++++-----------------------------
> 1 file changed, 8 insertions(+), 29 deletions(-)
> 
> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
> index eb6cd0e66..9d01d06ce 100644
> --- a/html/cgi-bin/zoneconf.cgi
> +++ b/html/cgi-bin/zoneconf.cgi
> @@ -25,6 +25,7 @@ use Scalar::Util qw(looks_like_number);
> require '/var/ipfire/general-functions.pl';
> require "${General::swroot}/lang.pl";
> require "${General::swroot}/header.pl";
> +require "${General::swroot}/network-functions.pl";
> 
> ###--- HTML HEAD ---###
> my $extraHead = <<END
> @@ -152,8 +153,8 @@ my $restart_notice = "";
> &Header::getcgihash(\%cgiparams);
> &Header::showhttpheaders();
> 
> -# Define all zones we will check for NIC assignment
> -my @zones = ("red", "green", "orange", "blue");
> +# Get all network zones that are currently enabled
> +my @zones = Network::get_available_network_zones();
> 
> # Get all physical NICs present
> opendir(my $dh, "/sys/class/net/");
> @@ -187,20 +188,6 @@ 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');
> @@ -211,7 +198,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> 	my %VALIDATE_nic_check = ();
> 	my $VALIDATE_error = "";
> 
> -	foreach (@zones) {
> +	# Loop trough all known zones to ensure a complete configuration file is created
> +	foreach (@Network::known_network_zones) {

Quite a good idea to have a global constant for this :)

> 		my $uc = uc $_;
> 		my $slave_string = "";
> 		my $zone_mode = $cgiparams{"MODE $uc"};
> @@ -383,14 +371,11 @@ END
> foreach (@zones) {
> 	my $uc = uc $_;
> 
> -	# If the zone is not activated, don't show it
> -	next unless is_zone_activated($_);
> -
> 	# If the red zone is in PPP mode, don't show a mode dropdown
> 	if ($uc eq "RED") {
> 		my $red_type = $ethsettings{"RED_TYPE"};
> 
> -		unless (is_zonetype_ip($red_type)) {
> +		unless (Network::is_red_mode_ip()) {

Wouldn’t perl complain if this isn’t called with an “&”?

> 			print "\t\t<td class='heading bold $_'>$uc ($red_type)</td>\n";
> 
> 			next; # We're done here
> @@ -436,12 +421,9 @@ foreach (@nics) {
> 		my $uc = uc $_;
> 		my $highlight = "";
> 
> -		# If the zone is not activated, don't show it
> -		next unless is_zone_activated($_);
> -
> 		if ($uc eq "RED") {
> 			# VLANs/Bridging is not possible if the RED interface is set to PPP, PPPoE, VDSL, ...
> -			unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
> +			unless (Network::is_red_mode_ip()) {
> 				my $checked = "";
> 
> 				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
> @@ -519,12 +501,9 @@ 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"})) {
> +		unless (Network::is_red_mode_ip()) {

Same as above.

> 			push(@stp_html, ["\t\t<td></td>\n", "\t\t<td></td>\n"]); # print empty cell
> 			next;
> 		}
> -- 
> 2.27.0.windows.1
> 

-Michael

Aw: Re: [PATCH v2 5/6] zoneconf.cgi: Import network-functions.pl

[Bernhard Bitsch]

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



> Gesendet: Freitag, 19. Februar 2021 um 20:24 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Leo-Andres Hofmann" <hofmann(a)leo-andres.de>
> Cc: development(a)lists.ipfire.org
> Betreff: Re: [PATCH v2 5/6] zoneconf.cgi: Import network-functions.pl
>
> 
> 
> > On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
> > 
> > Remove custom functions and use network-functions.pl instead to detect
> > the available zones correctly. This also removes the requirement that
> > a device must be assigned for a zone to become visible/configurable.
> > 
> > Fixes: #12568
> > 
> > Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> > ---
> > html/cgi-bin/zoneconf.cgi | 37 ++++++++-----------------------------
> > 1 file changed, 8 insertions(+), 29 deletions(-)
> > 
> > diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
> > index eb6cd0e66..9d01d06ce 100644
> > --- a/html/cgi-bin/zoneconf.cgi
> > +++ b/html/cgi-bin/zoneconf.cgi
> > @@ -25,6 +25,7 @@ use Scalar::Util qw(looks_like_number);
> > require '/var/ipfire/general-functions.pl';
> > require "${General::swroot}/lang.pl";
> > require "${General::swroot}/header.pl";
> > +require "${General::swroot}/network-functions.pl";
> > 
> > ###--- HTML HEAD ---###
> > my $extraHead = <<END
> > @@ -152,8 +153,8 @@ my $restart_notice = "";
> > &Header::getcgihash(\%cgiparams);
> > &Header::showhttpheaders();
> > 
> > -# Define all zones we will check for NIC assignment
> > -my @zones = ("red", "green", "orange", "blue");
> > +# Get all network zones that are currently enabled
> > +my @zones = Network::get_available_network_zones();
> > 
> > # Get all physical NICs present
> > opendir(my $dh, "/sys/class/net/");
> > @@ -187,20 +188,6 @@ 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');
> > @@ -211,7 +198,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> > 	my %VALIDATE_nic_check = ();
> > 	my $VALIDATE_error = "";
> > 
> > -	foreach (@zones) {
> > +	# Loop trough all known zones to ensure a complete configuration file is created
> > +	foreach (@Network::known_network_zones) {
> 
> Quite a good idea to have a global constant for this :)
> 
> > 		my $uc = uc $_;
> > 		my $slave_string = "";
> > 		my $zone_mode = $cgiparams{"MODE $uc"};
> > @@ -383,14 +371,11 @@ END
> > foreach (@zones) {
> > 	my $uc = uc $_;
> > 
> > -	# If the zone is not activated, don't show it
> > -	next unless is_zone_activated($_);
> > -
> > 	# If the red zone is in PPP mode, don't show a mode dropdown
> > 	if ($uc eq "RED") {
> > 		my $red_type = $ethsettings{"RED_TYPE"};
> > 
> > -		unless (is_zonetype_ip($red_type)) {
> > +		unless (Network::is_red_mode_ip()) {
> 
> Wouldn’t perl complain if this isn’t called with an “&”?
> 

No. This is the normal call now. The syntax with '&' brings some (minimal) restrictions.

> > 			print "\t\t<td class='heading bold $_'>$uc ($red_type)</td>\n";
> > 
> > 			next; # We're done here
> > @@ -436,12 +421,9 @@ foreach (@nics) {
> > 		my $uc = uc $_;
> > 		my $highlight = "";
> > 
> > -		# If the zone is not activated, don't show it
> > -		next unless is_zone_activated($_);
> > -
> > 		if ($uc eq "RED") {
> > 			# VLANs/Bridging is not possible if the RED interface is set to PPP, PPPoE, VDSL, ...
> > -			unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
> > +			unless (Network::is_red_mode_ip()) {
> > 				my $checked = "";
> > 
> > 				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
> > @@ -519,12 +501,9 @@ 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"})) {
> > +		unless (Network::is_red_mode_ip()) {
> 
> Same as above.
> 
> > 			push(@stp_html, ["\t\t<td></td>\n", "\t\t<td></td>\n"]); # print empty cell
> > 			next;
> > 		}
> > -- 
> > 2.27.0.windows.1
> > 
> 
> -Michael

Re: [PATCH v2 5/6] zoneconf.cgi: Import network-functions.pl

[Leo Hofmann]

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


Am 20.02.2021 um 12:07 schrieb Bernhard Bitsch:
>
>> Gesendet: Freitag, 19. Februar 2021 um 20:24 Uhr
>> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
>> An: "Leo-Andres Hofmann" <hofmann(a)leo-andres.de>
>> Cc: development(a)lists.ipfire.org
>> Betreff: Re: [PATCH v2 5/6] zoneconf.cgi: Import network-functions.pl
>>
>>
>>
>>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
>>>
>>> Remove custom functions and use network-functions.pl instead to detect
>>> the available zones correctly. This also removes the requirement that
>>> a device must be assigned for a zone to become visible/configurable.
>>>
>>> Fixes: #12568
>>>
>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>> ---
>>> html/cgi-bin/zoneconf.cgi | 37 ++++++++-----------------------------
>>> 1 file changed, 8 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>>> index eb6cd0e66..9d01d06ce 100644
>>> --- a/html/cgi-bin/zoneconf.cgi
>>> +++ b/html/cgi-bin/zoneconf.cgi
>>> @@ -25,6 +25,7 @@ use Scalar::Util qw(looks_like_number);
>>> require '/var/ipfire/general-functions.pl';
>>> require "${General::swroot}/lang.pl";
>>> require "${General::swroot}/header.pl";
>>> +require "${General::swroot}/network-functions.pl";
>>>
>>> ###--- HTML HEAD ---###
>>> my $extraHead = <<END
>>> @@ -152,8 +153,8 @@ my $restart_notice = "";
>>> &Header::getcgihash(\%cgiparams);
>>> &Header::showhttpheaders();
>>>
>>> -# Define all zones we will check for NIC assignment
>>> -my @zones = ("red", "green", "orange", "blue");
>>> +# Get all network zones that are currently enabled
>>> +my @zones = Network::get_available_network_zones();
>>>
>>> # Get all physical NICs present
>>> opendir(my $dh, "/sys/class/net/");
>>> @@ -187,20 +188,6 @@ 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');
>>> @@ -211,7 +198,8 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
>>> 	my %VALIDATE_nic_check = ();
>>> 	my $VALIDATE_error = "";
>>>
>>> -	foreach (@zones) {
>>> +	# Loop trough all known zones to ensure a complete configuration file is created
>>> +	foreach (@Network::known_network_zones) {
>> Quite a good idea to have a global constant for this :)
>>
>>> 		my $uc = uc $_;
>>> 		my $slave_string = "";
>>> 		my $zone_mode = $cgiparams{"MODE $uc"};
>>> @@ -383,14 +371,11 @@ END
>>> foreach (@zones) {
>>> 	my $uc = uc $_;
>>>
>>> -	# If the zone is not activated, don't show it
>>> -	next unless is_zone_activated($_);
>>> -
>>> 	# If the red zone is in PPP mode, don't show a mode dropdown
>>> 	if ($uc eq "RED") {
>>> 		my $red_type = $ethsettings{"RED_TYPE"};
>>>
>>> -		unless (is_zonetype_ip($red_type)) {
>>> +		unless (Network::is_red_mode_ip()) {
>> Wouldn’t perl complain if this isn’t called with an “&”?
>>
> No. This is the normal call now. The syntax with '&' brings some (minimal) restrictions.

I agree with Bernhard, that's how i understood it too.
As far as I know, you only have to use "&" if you want to call a function before you define it. Otherwise, the current advice is to actually avoid it.
Since I included the "Network" package right at the beginning, perl should already know about this function at this point.

>
>>> 			print "\t\t<td class='heading bold $_'>$uc ($red_type)</td>\n";
>>>
>>> 			next; # We're done here
>>> @@ -436,12 +421,9 @@ foreach (@nics) {
>>> 		my $uc = uc $_;
>>> 		my $highlight = "";
>>>
>>> -		# If the zone is not activated, don't show it
>>> -		next unless is_zone_activated($_);
>>> -
>>> 		if ($uc eq "RED") {
>>> 			# VLANs/Bridging is not possible if the RED interface is set to PPP, PPPoE, VDSL, ...
>>> -			unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
>>> +			unless (Network::is_red_mode_ip()) {
>>> 				my $checked = "";
>>>
>>> 				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
>>> @@ -519,12 +501,9 @@ 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"})) {
>>> +		unless (Network::is_red_mode_ip()) {
>> Same as above.
>>
>>> 			push(@stp_html, ["\t\t<td></td>\n", "\t\t<td></td>\n"]); # print empty cell
>>> 			next;
>>> 		}
>>> -- 
>>> 2.27.0.windows.1
>>>
>> -Michael

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Leo Hofmann]

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

Hello Michael,

thank you for looking into these patches. I'll answer below!

Am 19.02.2021 um 20:22 schrieb Michael Tremer:
> Hello,
>
>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
>>
>> Add default values and mark fields as required.
>>
>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>> ---
>> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>> index 9d01d06ce..bbd042ffc 100644
>> --- a/html/cgi-bin/zoneconf.cgi
>> +++ b/html/cgi-bin/zoneconf.cgi
>> @@ -478,6 +478,9 @@ END
>> 		if ($access_selected{"NONE"} eq "") {
>> 			$highlight = $_;
>> 		}
>> +		
>> +		# default VLAN tag if not configured
>> +		$zone_vlan_id = 1 unless looks_like_number($zone_vlan_id);
> I am not sure if it is a good idea to add a default here.
>
> Isn’t there a danger that people will hit save prematurely and connect the wrong VLAN with another one?
>
> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t make it easier to run into that error.

Agreed, this could happen if you are careless!
However, I hope you only enable VLAN if you are sure which ID you are supposed to use. In that case, having a default value appear makes it obvious where you need to put your desired ID.
But now I'm not sure about this one. I can revert this, of course.

>
>> 		print <<END
>> 		<td class="$highlight">
>> @@ -486,7 +489,7 @@ END
>> 				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
>> 				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
>> 			</select>
>> -			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
>> +			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" required $field_disabled>
>> 		</td>
>> END
>> ;
>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>> 	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"};
>> +	
>> +	# set priority to default value if no numerical value is configured
>> +	$stp_priority = 32768 unless looks_like_number($stp_priority);
> This is very good.
Thanks :)
>
> Since this is in this patchset and comes with the dependency to the other code above, I cannot pull this in with the STP patchset where it actually belongs.
>
> But I guess we should merge this all together anyways :)
I still have these commits in my local git. I could just submit these last two changes if that makes your job easier (and you don't mind me submitting everything multiple times)!
>
> -Michael
>
>> 	# form element modifiers
>> 	my $checked = "";
>> @@ -532,7 +538,7 @@ END
>> 	# priority input box HTML
>> 	my $row_2 = <<END
>> 		<td>
>> -			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" 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" required $disabled>
>> 		</td>
>> END
>> ;
>> -- 
>> 2.27.0.windows.1
>>
Regards, Leo

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Jonatan Schlag]

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

Hi,

first thanks for the patchset. Overall it looks quite good

Am Sonntag, den 21.02.2021, 11:38 +0100 schrieb Leo Hofmann:
> Hello Michael,
> 
> thank you for looking into these patches. I'll answer below!
> 
> Am 19.02.2021 um 20:22 schrieb Michael Tremer:
> > Hello,
> > 
> > > On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <
> > > hofmann(a)leo-andres.de> wrote:
> > > 
> > > Add default values and mark fields as required.
> > > 
> > > Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> > > ---
> > > html/cgi-bin/zoneconf.cgi | 10 ++++++++--
> > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-
> > > bin/zoneconf.cgi
> > > index 9d01d06ce..bbd042ffc 100644
> > > --- a/html/cgi-bin/zoneconf.cgi
> > > +++ b/html/cgi-bin/zoneconf.cgi
> > > @@ -478,6 +478,9 @@ END
> > > 		if ($access_selected{"NONE"} eq "") {
> > > 			$highlight = $_;
> > > 		}
> > > +		
> > > +		# default VLAN tag if not configured
> > > +		$zone_vlan_id = 1 unless
> > > looks_like_number($zone_vlan_id);
> > I am not sure if it is a good idea to add a default here.
> > 
> > Isn’t there a danger that people will hit save prematurely and
> > connect the wrong VLAN with another one?

This was also my first thought.
> > 
> > Usability issues like that cannot be prevented entirely, but I was
> > wondering if this didn’t make it easier to run into that error.
Thats definitly the case. 
> 
> Agreed, this could happen if you are careless!
> However, I hope you only enable VLAN if you are sure which ID you are
> supposed to use. In that case, having a default value appear makes it
> obvious where you need to put your desired ID.
> But now I'm not sure about this one. I can revert this, of course.

I would vote for without default value. There is just absolutey no save
default value here and people could assume that this is something like,
"I am clicking save and I am done". And thats not the case people have
to think about vlan ids. I unterstand that field might be hard to spot,
but that should not be solved by a default value. 

Another question
which came to my mind while reviewing this: Is there any good error
when the vlan id is wrong? Shure it is checked in the html, but i have
not found any backend check ... (Not a problem with your patch, but i
want to mention it.)

Apart from this linter is happy too.

Nice work.

Jonat
an

PS.: Apart from this reading the file took 10 mins, till I found out
that $uc has something todo with zones ... . At least a good learning
how to not name variables :-)


> 
> > > 		print <<END
> > > 		<td class="$highlight">
> > > @@ -486,7 +489,7 @@ END
> > > 				<option value="NATIVE"
> > > $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access
> > > native"}</option>
> > > 				<option value="VLAN"
> > > $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf
> > > access vlan"}</option>
> > > 			</select>
> > > -			<input type="number" class="vlanid" id="TAG-
> > > $uc-$mac" name="TAG $uc $mac" min="1" max="4095"
> > > value="$zone_vlan_id" $field_disabled>
> > > +			<input type="number" class="vlanid" id="TAG-
> > > $uc-$mac" name="TAG $uc $mac" min="1" max="4095"
> > > value="$zone_vlan_id" required $field_disabled>
> > > 		</td>
> > > END
> > > ;
> > > @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and
> > > prepare form elements for each zone
> > > 	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"};
> > > +	
> > > +	# set priority to default value if no numerical value is
> > > configured
> > > +	$stp_priority = 32768 unless looks_like_number($stp_priority);
> > This is very good.
> Thanks :)
> > Since this is in this patchset and comes with the dependency to the
> > other code above, I cannot pull this in with the STP patchset where
> > it actually belongs.
> > 
> > But I guess we should merge this all together anyways :)
> I still have these commits in my local git. I could just submit these
> last two changes if that makes your job easier (and you don't mind me
> submitting everything multiple times)!
> > -Michael
> > 
> > > 	# form element modifiers
> > > 	my $checked = "";
> > > @@ -532,7 +538,7 @@ END
> > > 	# priority input box HTML
> > > 	my $row_2 = <<END
> > > 		<td>
> > > -			<input type="number" class="stp-priority"
> > > id="STP-PRIORITY-$uc" 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" required $disabled>
> > > 		</td>
> > > END
> > > ;
> > > -- 
> > > 2.27.0.windows.1
> > > 
> Regards, Leo

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Michael Tremer]

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

Hi,

> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
> 
> Hello Michael,
> 
> thank you for looking into these patches. I'll answer below!
> 
> Am 19.02.2021 um 20:22 schrieb Michael Tremer:
>> Hello,
>> 
>>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
>>> 
>>> Add default values and mark fields as required.
>>> 
>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>> ---
>>> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>>> index 9d01d06ce..bbd042ffc 100644
>>> --- a/html/cgi-bin/zoneconf.cgi
>>> +++ b/html/cgi-bin/zoneconf.cgi
>>> @@ -478,6 +478,9 @@ END
>>> 		if ($access_selected{"NONE"} eq "") {
>>> 			$highlight = $_;
>>> 		}
>>> +		
>>> +		# default VLAN tag if not configured
>>> +		$zone_vlan_id = 1 unless looks_like_number($zone_vlan_id);
>> I am not sure if it is a good idea to add a default here.
>> 
>> Isn’t there a danger that people will hit save prematurely and connect the wrong VLAN with another one?
>> 
>> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t make it easier to run into that error.
> 
> Agreed, this could happen if you are careless!
> However, I hope you only enable VLAN if you are sure which ID you are supposed to use. In that case, having a default value appear makes it obvious where you need to put your desired ID.
> But now I'm not sure about this one. I can revert this, of course.

Well, we do not have many votes, but let’s maybe leave it empty by default.

I will merge the patch and edit this change out.

>> 
>>> 		print <<END
>>> 		<td class="$highlight">
>>> @@ -486,7 +489,7 @@ END
>>> 				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
>>> 				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
>>> 			</select>
>>> -			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
>>> +			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" required $field_disabled>
>>> 		</td>
>>> END
>>> ;
>>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>>> 	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"};
>>> +	
>>> +	# set priority to default value if no numerical value is configured
>>> +	$stp_priority = 32768 unless looks_like_number($stp_priority);
>> This is very good.
> Thanks :)
>> 
>> Since this is in this patchset and comes with the dependency to the other code above, I cannot pull this in with the STP patchset where it actually belongs.
>> 
>> But I guess we should merge this all together anyways :)
> I still have these commits in my local git. I could just submit these last two changes if that makes your job easier (and you don't mind me submitting everything multiple times)!

No need for such a small thing.

But thanks anyways :)

-Michael

>> 
>> -Michael
>> 
>>> 	# form element modifiers
>>> 	my $checked = "";
>>> @@ -532,7 +538,7 @@ END
>>> 	# priority input box HTML
>>> 	my $row_2 = <<END
>>> 		<td>
>>> -			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" 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" required $disabled>
>>> 		</td>
>>> END
>>> ;
>>> -- 
>>> 2.27.0.windows.1
>>> 
> Regards, Leo

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Adolf Belka (ipfire)]

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

Hi all,

On 22/02/2021 20:02, Michael Tremer wrote:
> Hi,
>
>> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
>>
>> Hello Michael,
>>
>> thank you for looking into these patches. I'll answer below!
>>
>> Am 19.02.2021 um 20:22 schrieb Michael Tremer:
>>> Hello,
>>>
>>>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>>> ---
>>>> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>>>> index 9d01d06ce..bbd042ffc 100644
>>>> --- a/html/cgi-bin/zoneconf.cgi
>>>> +++ b/html/cgi-bin/zoneconf.cgi
>>>> @@ -478,6 +478,9 @@ END
>>>> 		if ($access_selected{"NONE"} eq "") {
>>>> 			$highlight = $_;
>>>> 		}
>>>> +		
>>>> +		# default VLAN tag if not configured
>>>> +		$zone_vlan_id = 1 unless looks_like_number($zone_vlan_id);
>>> I am not sure if it is a good idea to add a default here.
>>>
>>> Isn’t there a danger that people will hit save prematurely and connect the wrong VLAN with another one?
>>>
>>> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t make it easier to run into that error.
>> Agreed, this could happen if you are careless!
>> However, I hope you only enable VLAN if you are sure which ID you are supposed to use. In that case, having a default value appear makes it obvious where you need to put your desired ID.
>> But now I'm not sure about this one. I can revert this, of course.
> Well, we do not have many votes, but let’s maybe leave it empty by default.

Sorry for late reply. For me, I had no problem figuring out where I should put the VLAN ID. I am happy with the system as it was but no hard feelings against having a default value either.

A decision to stay with empty by default is fine with me.


Regards,

Adolf

> I will merge the patch and edit this change out.
>
>>>> 		print <<END
>>>> 		<td class="$highlight">
>>>> @@ -486,7 +489,7 @@ END
>>>> 				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
>>>> 				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
>>>> 			</select>
>>>> -			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
>>>> +			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" required $field_disabled>
>>>> 		</td>
>>>> END
>>>> ;
>>>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>>>> 	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"};
>>>> +	
>>>> +	# set priority to default value if no numerical value is configured
>>>> +	$stp_priority = 32768 unless looks_like_number($stp_priority);
>>> This is very good.
>> Thanks :)
>>> Since this is in this patchset and comes with the dependency to the other code above, I cannot pull this in with the STP patchset where it actually belongs.
>>>
>>> But I guess we should merge this all together anyways :)
>> I still have these commits in my local git. I could just submit these last two changes if that makes your job easier (and you don't mind me submitting everything multiple times)!
> No need for such a small thing.
>
> But thanks anyways :)
>
> -Michael
>
>>> -Michael
>>>
>>>> 	# form element modifiers
>>>> 	my $checked = "";
>>>> @@ -532,7 +538,7 @@ END
>>>> 	# priority input box HTML
>>>> 	my $row_2 = <<END
>>>> 		<td>
>>>> -			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" 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" required $disabled>
>>>> 		</td>
>>>> END
>>>> ;
>>>> -- 
>>>> 2.27.0.windows.1
>>>>
>> Regards, Leo

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Leo Hofmann]

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

Hi,

Am 22.02.2021 um 20:02 schrieb Michael Tremer:
> Hi,
>
>> On 21 Feb 2021, at 10:38, Leo Hofmann <hofmann(a)leo-andres.de> wrote:
>>
>> Hello Michael,
>>
>> thank you for looking into these patches. I'll answer below!
>>
>> Am 19.02.2021 um 20:22 schrieb Michael Tremer:
>>> Hello,
>>>
>>>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>>> ---
>>>> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-bin/zoneconf.cgi
>>>> index 9d01d06ce..bbd042ffc 100644
>>>> --- a/html/cgi-bin/zoneconf.cgi
>>>> +++ b/html/cgi-bin/zoneconf.cgi
>>>> @@ -478,6 +478,9 @@ END
>>>> 		if ($access_selected{"NONE"} eq "") {
>>>> 			$highlight = $_;
>>>> 		}
>>>> +		
>>>> +		# default VLAN tag if not configured
>>>> +		$zone_vlan_id = 1 unless looks_like_number($zone_vlan_id);
>>> I am not sure if it is a good idea to add a default here.
>>>
>>> Isn’t there a danger that people will hit save prematurely and connect the wrong VLAN with another one?
>>>
>>> Usability issues like that cannot be prevented entirely, but I was wondering if this didn’t make it easier to run into that error.
>> Agreed, this could happen if you are careless!
>> However, I hope you only enable VLAN if you are sure which ID you are supposed to use. In that case, having a default value appear makes it obvious where you need to put your desired ID.
>> But now I'm not sure about this one. I can revert this, of course.
> Well, we do not have many votes, but let’s maybe leave it empty by default.
I just read Jonatan's email and was about to write that I agree with both of you, but you beat me to it.
>
> I will merge the patch and edit this change out.

Awesome, I didn't know you could do that. Thanks, this saves me from submitting this a third time :D

Best,
Leo

>
>>>> 		print <<END
>>>> 		<td class="$highlight">
>>>> @@ -486,7 +489,7 @@ END
>>>> 				<option value="NATIVE" $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access native"}</option>
>>>> 				<option value="VLAN" $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf access vlan"}</option>
>>>> 			</select>
>>>> -			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" $field_disabled>
>>>> +			<input type="number" class="vlanid" id="TAG-$uc-$mac" name="TAG $uc $mac" min="1" max="4095" value="$zone_vlan_id" required $field_disabled>
>>>> 		</td>
>>>> END
>>>> ;
>>>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form elements for each zone
>>>> 	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"};
>>>> +	
>>>> +	# set priority to default value if no numerical value is configured
>>>> +	$stp_priority = 32768 unless looks_like_number($stp_priority);
>>> This is very good.
>> Thanks :)
>>> Since this is in this patchset and comes with the dependency to the other code above, I cannot pull this in with the STP patchset where it actually belongs.
>>>
>>> But I guess we should merge this all together anyways :)
>> I still have these commits in my local git. I could just submit these last two changes if that makes your job easier (and you don't mind me submitting everything multiple times)!
> No need for such a small thing.
>
> But thanks anyways :)
>
> -Michael
>
>>> -Michael
>>>
>>>> 	# form element modifiers
>>>> 	my $checked = "";
>>>> @@ -532,7 +538,7 @@ END
>>>> 	# priority input box HTML
>>>> 	my $row_2 = <<END
>>>> 		<td>
>>>> -			<input type="number" class="stp-priority" id="STP-PRIORITY-$uc" 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" required $disabled>
>>>> 		</td>
>>>> END
>>>> ;
>>>> -- 
>>>> 2.27.0.windows.1
>>>>
>> Regards, Leo

Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs

[Leo Hofmann]

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

Hi Jonatan,

thank you for looking into this, I'll answer below!

Am 21.02.2021 um 20:06 schrieb Jonatan Schlag:
> Hi,
>
> first thanks for the patchset. Overall it looks quite good
>
> Am Sonntag, den 21.02.2021, 11:38 +0100 schrieb Leo Hofmann:
>> Hello Michael,
>>
>> thank you for looking into these patches. I'll answer below!
>>
>> Am 19.02.2021 um 20:22 schrieb Michael Tremer:
>>> Hello,
>>>
>>>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann <
>>>> hofmann(a)leo-andres.de> wrote:
>>>>
>>>> Add default values and mark fields as required.
>>>>
>>>> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
>>>> ---
>>>> html/cgi-bin/zoneconf.cgi | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi-
>>>> bin/zoneconf.cgi
>>>> index 9d01d06ce..bbd042ffc 100644
>>>> --- a/html/cgi-bin/zoneconf.cgi
>>>> +++ b/html/cgi-bin/zoneconf.cgi
>>>> @@ -478,6 +478,9 @@ END
>>>> 		if ($access_selected{"NONE"} eq "") {
>>>> 			$highlight = $_;
>>>> 		}
>>>> +		
>>>> +		# default VLAN tag if not configured
>>>> +		$zone_vlan_id = 1 unless
>>>> looks_like_number($zone_vlan_id);
>>> I am not sure if it is a good idea to add a default here.
>>>
>>> Isn’t there a danger that people will hit save prematurely and
>>> connect the wrong VLAN with another one?
> This was also my first thought.
>>> Usability issues like that cannot be prevented entirely, but I was
>>> wondering if this didn’t make it easier to run into that error.
> Thats definitly the case.
>> Agreed, this could happen if you are careless!
>> However, I hope you only enable VLAN if you are sure which ID you are
>> supposed to use. In that case, having a default value appear makes it
>> obvious where you need to put your desired ID.
>> But now I'm not sure about this one. I can revert this, of course.
> I would vote for without default value. There is just absolutey no save
> default value here and people could assume that this is something like,
> "I am clicking save and I am done". And thats not the case people have
> to think about vlan ids. I unterstand that field might be hard to spot,
> but that should not be solved by a default value.
>
> Another question
> which came to my mind while reviewing this: Is there any good error
> when the vlan id is wrong? Shure it is checked in the html, but i have
> not found any backend check ... (Not a problem with your patch, but i
> want to mention it.)
I just tried this on my test system. There is a range check but no error message.
Instead, it reverts the entire assignment, removes the NIC from the zone and saves the configuration like this. Well, now what...

I don't think this is very critical right now because the HTML range check will usually prevent this. But I think we should have a proper check in the backend at some point.
I'll put it on my list.

>
> Apart from this linter is happy too.
>
> Nice work.
>
> Jonat
> an
>
> PS.: Apart from this reading the file took 10 mins, till I found out
> that $uc has something todo with zones ... . At least a good learning
> how to not name variables :-)

$uc is the upper case zone name (RED, ..). This is already used everywhere, so I decided to keep it. I can put this on my list too.

Best regards
Leo

>
>
>>>> 		print <<END
>>>> 		<td class="$highlight">
>>>> @@ -486,7 +489,7 @@ END
>>>> 				<option value="NATIVE"
>>>> $access_selected{"NATIVE"}>$Lang::tr{"zoneconf access
>>>> native"}</option>
>>>> 				<option value="VLAN"
>>>> $access_selected{"VLAN"} $vlan_disabled>$Lang::tr{"zoneconf
>>>> access vlan"}</option>
>>>> 			</select>
>>>> -			<input type="number" class="vlanid" id="TAG-
>>>> $uc-$mac" name="TAG $uc $mac" min="1" max="4095"
>>>> value="$zone_vlan_id" $field_disabled>
>>>> +			<input type="number" class="vlanid" id="TAG-
>>>> $uc-$mac" name="TAG $uc $mac" min="1" max="4095"
>>>> value="$zone_vlan_id" required $field_disabled>
>>>> 		</td>
>>>> END
>>>> ;
>>>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and
>>>> prepare form elements for each zone
>>>> 	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"};
>>>> +	
>>>> +	# set priority to default value if no numerical value is
>>>> configured
>>>> +	$stp_priority = 32768 unless looks_like_number($stp_priority);
>>> This is very good.
>> Thanks :)
>>> Since this is in this patchset and comes with the dependency to the
>>> other code above, I cannot pull this in with the STP patchset where
>>> it actually belongs.
>>>
>>> But I guess we should merge this all together anyways :)
>> I still have these commits in my local git. I could just submit these
>> last two changes if that makes your job easier (and you don't mind me
>> submitting everything multiple times)!
>>> -Michael
>>>
>>>> 	# form element modifiers
>>>> 	my $checked = "";
>>>> @@ -532,7 +538,7 @@ END
>>>> 	# priority input box HTML
>>>> 	my $row_2 = <<END
>>>> 		<td>
>>>> -			<input type="number" class="stp-priority"
>>>> id="STP-PRIORITY-$uc" 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" required $disabled>
>>>> 		</td>
>>>> END
>>>> ;
>>>> -- 
>>>> 2.27.0.windows.1
>>>>
>> Regards, Leo

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

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