From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 1/4] zoneconf.cgi: Change NIC display order, improve code
Date: Sun, 27 Dec 2020 12:04:47 +0100
Message-ID: <9354B4F0-7931-41E5-AE8D-87191ABEA148@ipfire.org>
In-Reply-To: <20201222200646.1609-1-hofmann@leo-andres.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2855554580066603801=="
List-Id: <development.lists.ipfire.org>

--===============2855554580066603801==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi,

> On 22 Dec 2020, at 21:06, Leo-Andres Hofmann <hofmann(a)leo-andres.de> wrot=
e:
>=20
> Refactor duplicate perl code and add comments
>=20
> Signed-off-by: Leo-Andres Hofmann <hofmann(a)leo-andres.de>
> ---
> html/cgi-bin/zoneconf.cgi | 53 +++++++++++++++++++++++++--------------
> 1 file changed, 34 insertions(+), 19 deletions(-)
>=20
> 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";
>=20
> +###--- HTML HEAD ---###
> my $extraHead =3D <<END
> <style>
> 	table#zoneconf {
> @@ -105,7 +106,9 @@ my $extraHead =3D <<END
> <script src=3D"/include/zoneconf.js"></script>
> END
> ;
> +###--- END HTML HEAD ---###
>=20
> +### Read configuration ###
> my %ethsettings =3D ();
> my %vlansettings =3D ();
> my %cgiparams =3D ();
> @@ -119,7 +122,7 @@ my $restart_notice =3D "";
> &Header::showhttpheaders();
>=20
> # Define all zones we will check for NIC assignment
> -my @zones =3D ("green", "red", "orange", "blue");
> +my @zones =3D ("red", "green", "orange", "blue");
>=20
> # Get all physical NICs present
> opendir(my $dh, "/sys/class/net/");
> @@ -153,6 +156,21 @@ foreach (@nics) {
> 	}
> }
>=20
> +### Functions ###
> +
> +# Check if a zone is in IP mode or in PPP, PPPoE, VDSL, ... mode
> +sub is_zonetype_ip {
> +	my $zone_type =3D shift;
> +	return ($zone_type eq "STATIC" || $zone_type eq "DHCP");
> +}
> +
> +# Check if a zone is activated (device assigned)
> +sub is_zone_activated {
> +	my $zone =3D 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');
>=20
> @@ -195,6 +213,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> 				}
> 			}
>=20
> +			# skip NIC/VLAN assignment and additional zone options for RED in PPP m=
ode
> 			next;
> 		}

I like well-commented code.

> @@ -278,6 +297,7 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> 		}
> 	}
>=20
> +	# validation failed, show error message and exit
> 	if ($VALIDATE_error) {
> 		&Header::openbox('100%', 'left', $Lang::tr{"error"});
>=20
> @@ -290,16 +310,17 @@ if ($cgiparams{"ACTION"} eq $Lang::tr{"save"}) {
> 		exit 0;
> 	}
>=20
> +	# new settings are valid, write configuration files
> 	&General::writehash("${General::swroot}/ethernet/settings",\%ethsettings);
> 	&General::writehash("${General::swroot}/ethernet/vlans",\%vlansettings);
>=20
> 	$restart_notice =3D $Lang::tr{'zoneconf notice reboot'};
> }
>=20
> -&Header::openbox('100%', 'left', $Lang::tr{"zoneconf nic assignment"});
> -
> ### START OF TABLE ###
>=20
> +&Header::openbox('100%', 'left', $Lang::tr{"zoneconf nic assignment"});
> +
> print <<END
> <form method=3D'post' enctype=3D'multipart/form-data'>
> 	<table id=3D"zoneconf">
> @@ -311,19 +332,16 @@ END
> # Fill the table header with all activated zones
> foreach (@zones) {
> 	my $uc =3D uc $_;
> -	my $dev_name =3D $ethsettings{"${uc}_DEV"};
>=20
> -	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($_);
>=20
> -	# 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 =3D $ethsettings{"RED_TYPE"};
> -		my $red_restricted =3D ($uc eq "RED" && ! ($red_type eq "STATIC" || $red=
_type eq "DHCP"));
>=20
> -		if ($red_restricted) {
> -			print "\t\t<td class=3D'heading $_'>$uc ($red_type)</td>\n";
> +		unless (is_zonetype_ip($red_type)) {
> +			print "\t\t<td class=3D'heading bold $_'>$uc ($red_type)</td>\n";
>=20
> 			next; # We're done here
> 		}
> @@ -354,6 +372,7 @@ END
>=20
> print "\t</tr>\n";
>=20
> +# NIC assignment matrix
> foreach (@nics) {
> 	my $mac =3D $_->[0];
> 	my $nic =3D $_->[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 =3D uc $_;
> -		my $dev_name =3D $ethsettings{"${uc}_DEV"};
> 		my $highlight =3D "";
>=20
> -		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 =3D $ethsettings{"RED_TYPE"};
> -			my $red_restricted =3D ($uc eq "RED" && ! ($red_type eq "STATIC" || $re=
d_type eq "DHCP"));
> -
> 			# VLANs/Bridging is not possible if the RED interface is set to PPP, PPP=
oE, VDSL, ...
> -			if ($red_restricted) {
> +			unless (is_zonetype_ip($ethsettings{"RED_TYPE"})) {
> 				my $checked =3D "";
>=20
> 				if ($mac eq $ethsettings{"${uc}_MACADDR"}) {
> @@ -449,6 +463,7 @@ END
> 	print "\t</tr>\n";
> }
>=20
> +# footer and submit button
> print <<END
> 	</table>
>=20
> --=20
> 2.27.0.windows.1
>=20


--===============2855554580066603801==--