Refactor duplicate perl code and add comments
Signed-off-by: Leo-Andres Hofmann hofmann@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>
Simplify borders, load more colors from header and add dividers
Signed-off-by: Leo-Andres Hofmann hofmann@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) {
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@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',
Signed-off-by: Leo-Andres Hofmann hofmann@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); + } +}
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@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; }
On 18 Feb 2021, at 14:30, Leo-Andres Hofmann hofmann@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@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 itnext 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
Gesendet: Freitag, 19. Februar 2021 um 20:24 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Leo-Andres Hofmann" hofmann@leo-andres.de Cc: development@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@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@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 itnext 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
Am 20.02.2021 um 12:07 schrieb Bernhard Bitsch:
Gesendet: Freitag, 19. Februar 2021 um 20:24 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Leo-Andres Hofmann" hofmann@leo-andres.de Cc: development@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@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@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 itnext 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
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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 ;
Hello,
On 18 Feb 2021, at 14:30, Leo-Andres Hofmann hofmann@leo-andres.de wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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
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@leo-andres.de wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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
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@leo-andres.de> wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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 unlesslooks_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
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@leo-andres.de> wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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 unlesslooks_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
Hi,
On 21 Feb 2021, at 10:38, Leo Hofmann hofmann@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@leo-andres.de wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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
Hi all,
On 22/02/2021 20:02, Michael Tremer wrote:
Hi,
On 21 Feb 2021, at 10:38, Leo Hofmann hofmann@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@leo-andres.de wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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
Hi,
Am 22.02.2021 um 20:02 schrieb Michael Tremer:
Hi,
On 21 Feb 2021, at 10:38, Leo Hofmann hofmann@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@leo-andres.de wrote:
Add default values and mark fields as required.
Signed-off-by: Leo-Andres Hofmann hofmann@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
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!
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.
Hello,
On 18 Feb 2021, at 14:36, Leo Hofmann hofmann@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.
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@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.
*thumbs up*
Couldn’t agree more.
On 21 Feb 2021, at 12:14, Leo Hofmann hofmann@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@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.
-
Adolf Belka (ipfire) -
Bernhard Bitsch -
Jonatan Schlag -
Leo Hofmann -
Leo-Andres Hofmann
-
Michael Tremer