[1/4] zoneconf.cgi: Change NIC display order, improve code
Commit Message
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(-)
Comments
Hi,
> On 22 Dec 2020, at 21:06, Leo-Andres Hofmann <hofmann@leo-andres.de> wrote:
>
> 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 "");
> +}
Should we not have functions like this in network-functions.pl, so that we do not have to rewrite them every time?
We also have functions for this in general-functions.pl which use the CONFIG_TYPE setting from /var/ipfire/ethernet/settings. That should actually be the correct way to do this.
Maybe this is one for the list of things to tidy up :)
> +
> +### START PAGE ###
> &Header::openpage($Lang::tr{"zoneconf title"}, 1, $extraHead);
> &Header::openbigbox('100%', 'center');
>
> @@ -195,6 +213,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> }
> }
>
> + # skip NIC/VLAN assignment and additional zone options for RED in PPP mode
> next;
> }
I like well-commented code.
> @@ -278,6 +297,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> }
> }
>
> + # validation failed, show error message and exit
> if ($VALIDATE_error) {
> &Header::openbox('100%', 'left', $Lang::tr{"error"});
>
> @@ -290,16 +310,17 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> exit 0;
> }
>
> + # new settings are valid, write configuration files
> &General::writehash("${General::swroot}/ethernet/settings",\%ethsettings);
> &General::writehash("${General::swroot}/ethernet/vlans",\%vlansettings);
>
> $restart_notice = $Lang::tr{'zoneconf notice reboot'};
> }
>
> -&Header::openbox('100%', 'left', $Lang::tr{"zoneconf nic assignment"});
> -
> ### START OF TABLE ###
>
> +&Header::openbox('100%', 'left', $Lang::tr{"zoneconf nic assignment"});
> +
> print <<END
> <form method='post' enctype='multipart/form-data'>
> <table id="zoneconf">
> @@ -311,19 +332,16 @@ END
> # Fill the table header with all activated zones
> foreach (@zones) {
> my $uc = uc $_;
> - my $dev_name = $ethsettings{"${uc}_DEV"};
>
> - if ($dev_name eq "") { # If the zone is not activated, don't show it
> - next;
> - }
> + # If the zone is not activated, don't show it
> + next unless is_zone_activated($_);
>
> - # If the zone is in PPP mode, don't show a mode dropdown
> + # If the red zone is in PPP mode, don't show a mode dropdown
> if ($uc eq "RED") {
> my $red_type = $ethsettings{"RED_TYPE"};
> - my $red_restricted = ($uc eq "RED" && ! ($red_type eq "STATIC" || $red_type eq "DHCP"));
>
> - if ($red_restricted) {
> - print "\t\t<td class='heading $_'>$uc ($red_type)</td>\n";
> + unless (is_zonetype_ip($red_type)) {
> + print "\t\t<td class='heading bold $_'>$uc ($red_type)</td>\n";
>
> next; # We're done here
> }
> @@ -354,6 +372,7 @@ END
>
> print "\t</tr>\n";
>
> +# NIC assignment matrix
> foreach (@nics) {
> my $mac = $_->[0];
> my $nic = $_->[1];
> @@ -365,19 +384,14 @@ foreach (@nics) {
> # Iterate through all zones and check if the current NIC is assigned to it
> foreach (@zones) {
> my $uc = uc $_;
> - my $dev_name = $ethsettings{"${uc}_DEV"};
> my $highlight = "";
>
> - if ($dev_name eq "") { # Again, skip the zone if it is not activated
> - next;
> - }
> + # If the zone is not activated, don't show it
> + next unless is_zone_activated($_);
And this is a good change, because it clearly says what the code is supposed to do.
> if ($uc eq "RED") {
> - my $red_type = $ethsettings{"RED_TYPE"};
> - my $red_restricted = ($uc eq "RED" && ! ($red_type eq "STATIC" || $red_type eq "DHCP"));
> -
> # VLANs/Bridging is not possible if the RED interface is set to PPP, PPPoE, VDSL, ...
> - if ($red_restricted) {
> + unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
> my $checked = "";
>
> if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
> @@ -449,6 +463,7 @@ END
> print "\t</tr>\n";
> }
>
> +# footer and submit button
> print <<END
> </table>
>
> --
> 2.27.0.windows.1
>
@@ -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>