From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter =?utf-8?q?M=C3=BCller?= <peter.mueller@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] rules.pl: Adjust check against loading the same lists
 multiple times.
Date: Thu, 17 Feb 2022 19:25:03 +0000
Message-ID: <10606522-7857-84ca-5c88-749b09a7fe06@ipfire.org>
In-Reply-To: <20220217054003.7080-1-stefan.schantl@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8852377764037041311=="
List-Id: <development.lists.ipfire.org>

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

Reviewed-by: Peter M=C3=BCller <peter.mueller(a)ipfire.org>

> This check now has been moved to the ipset_restore() function, which
> will help to keep the code clean and maintain-able.
>=20
> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> ---
>  config/firewall/rules.pl | 43 ++++++++++++++++------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
>=20
> diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl
> index 25d01e0e3..927c1f2ba 100644
> --- a/config/firewall/rules.pl
> +++ b/config/firewall/rules.pl
> @@ -404,14 +404,8 @@ sub buildrules {
>  						# Grab location code from hash.
>  						my $loc_src =3D $$hash{$key}[4];
> =20
> -						# Check if the network list for this country already has been loaded.
> -						unless($loaded_ipset_lists{$loc_src}) {
> -							# Call function to load the networks list for this country.
> -							&ipset_restore($loc_src);
> -
> -							# Store to the hash that this list has been loaded.
> -							$loaded_ipset_lists{$loc_src} =3D "1";
> -						}
> +						# Call function to load the networks list for this country.
> +						&ipset_restore($loc_src);
> =20
>  						push(@source_options, $source);
>  					} elsif($source) {
> @@ -424,14 +418,8 @@ sub buildrules {
>  						# Grab location code from hash.
>  						my $loc_dst =3D $$hash{$key}[6];
> =20
> -						# Check if the network list for this country already has been loaded.
> -						unless($loaded_ipset_lists{$loc_dst}) {
> -							# Call function to load the networks list for this country.
> -							&ipset_restore($loc_dst);
> -
> -							# Store to the hash that this list has been loaded.
> -							$loaded_ipset_lists{$loc_dst} =3D "1";
> -						}
> +						# Call function to load the networks list for this country.
> +						&ipset_restore($loc_dst);
> =20
>  						push(@destination_options,  $destination);
>  					} elsif ($destination) {
> @@ -677,14 +665,8 @@ sub locationblock {
>  	# is enabled.
>  	foreach my $location (@locations) {
>  		if(exists $locationsettings{$location} && $locationsettings{$location} e=
q "on") {
> -			# Check if the network list for this country already has been loaded.
> -			unless($loaded_ipset_lists{$location}) {
> -				# Call function to load the networks list for this country.
> -				&ipset_restore($location);
> -
> -				# Store to the hash that this list has been loaded.
> -				$loaded_ipset_lists{$location} =3D "1";
> -			}
> +			# Call function to load the networks list for this country.
> +			&ipset_restore($location);
> =20
>  			# Call iptables and create rule to use the loaded ipset list.
>  			run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j =
DROP");
> @@ -906,14 +888,23 @@ sub firewall_is_in_subnet {
>  }
> =20
>  sub ipset_restore ($) {
> -	my ($ccode) =3D @_;
> +	my ($list) =3D @_;
> =20
>  	my $file_prefix =3D "ipset4";
> -	my $db_file =3D "$Location::Functions::ipset_db_directory/$ccode.$file_pr=
efix";
> +	my $db_file =3D "$Location::Functions::ipset_db_directory/$list.$file_pre=
fix";
> +
> +	# Check if the network list already has been loaded.
> +	if($loaded_ipset_lists{$list}) {
> +		# It already has been loaded - so there is nothing to do.
> +		return;
> +	}
> =20
>  	# Check if the generated file exists.
>  	if (-f $db_file) {
>  		# Run ipset and restore the list of the given country code.
>  		run("$IPSET restore < $db_file");
> +
> +		# Store the restored list name to the hash to prevent from loading it ag=
ain.
> +		$loaded_ipset_lists{$list} =3D "1";
>  	}
>  }

--===============8852377764037041311==--