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 09/12] rules.pl: Do not try to restore the same ipset
 multiple times.
Date: Tue, 15 Feb 2022 12:39:42 +0000
Message-ID: <4657F2F3-0D35-4232-88EC-B6B4DDD5BCEB@ipfire.org>
In-Reply-To: <20220214184257.2406-9-stefan.schantl@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============6254718058159658002=="
List-Id: <development.lists.ipfire.org>

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

Hello,

I would have implemented this differently.

Would it not be better to perform the check in ipset_restore() so that you wo=
n=E2=80=99t have to copy the code to everywhere you call ipset_restore?

This solution bloats the code slightly.

-Michael

> On 14 Feb 2022, at 18:42, Stefan Schantl <stefan.schantl(a)ipfire.org> wrot=
e:
>=20
> When an ipset list get restored, this now will be documented in a hash
> and this hash also will be checked before restoring a list if this has
> not be done previously.
>=20
> This will prevent from restoring the same list multiple times.
>=20
> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> ---
> config/firewall/rules.pl | 31 +++++++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
>=20
> diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl
> index d533ffb42..29990ee67 100644
> --- a/config/firewall/rules.pl
> +++ b/config/firewall/rules.pl
> @@ -70,6 +70,7 @@ my %confignatfw=3D();
> my %locationsettings =3D (
> 	"LOCATIONBLOCK_ENABLED" =3D> "off"
> );
> +my %loaded_ipset_lists=3D();
>=20
> my @p2ps=3D();
>=20
> @@ -405,8 +406,14 @@ sub buildrules {
> 						# Grab location code from hash.
> 						my $loc_src =3D $$hash{$key}[4];
>=20
> -						# Call function to load the networks list for this country.
> -						&ipset_restore($loc_src);
> +						# 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";
> +						}
>=20
> 						push(@source_options, $source);
> 					} elsif($source) {
> @@ -419,8 +426,14 @@ sub buildrules {
> 						# Grab location code from hash.
> 						my $loc_dst =3D $$hash{$key}[6];
>=20
> -						# Call function to load the networks list for this country.
> -						&ipset_restore($loc_dst);
> +						# 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";
> +						}
>=20
> 						push(@destination_options,  $destination);
> 					} elsif ($destination) {
> @@ -683,8 +696,14 @@ sub locationblock {
> 	# is enabled.
> 	foreach my $location (@locations) {
> 		if(exists $locationsettings{$location} && $locationsettings{$location} eq=
 "on") {
> -			# Call function to load the networks list for this country.
> -			&ipset_restore($location);
> +			# 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";
> +			}
>=20
> 			# Call iptables and create rule to use the loaded ipset list.
> 			run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j D=
ROP");
> --=20
> 2.30.2
>=20


--===============6254718058159658002==--