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

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hi,

> On 6 Jan 2021, at 13:20, Daniel Weismüller <daniel.weismueller(a)ipfire.org> wrote:
> 
> Hi,
> 
> no problems after applying all four patches so far. 
> Everything works as expected. 
> Further tests are planned for the next days.
> 
> But there is one thing that we should append.
> 
> If there is no priority configured the priority should be set to 32768.

Yes, currently the configuration cannot be saved without putting something into that field (as it should be). But it is not obvious for the user that this should be the case. I would also recommend setting the “required” attribute to the input fields.

Apart from that, I can also say that this looks good to be merged.

-Michael

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