[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