Hello Michael,
thanks for reviewing and your feedback.
You are absolutely right, it would give us much cleaner code when moving this kind of check into the ipset_restore() function.
I'll send a patch for this.
Best regards,
-Stefan
Hello,
I would have implemented this differently.
Would it not be better to perform the check in ipset_restore() so that you won’t 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@ipfire.org wrote:
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.
This will prevent from restoring the same list multiple times.
Signed-off-by: Stefan Schantl stefan.schantl@ipfire.org
config/firewall/rules.pl | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)
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=(); my %locationsettings = ( "LOCATIONBLOCK_ENABLED" => "off" ); +my %loaded_ipset_lists=();
my @p2ps=();
@@ -405,8 +406,14 @@ sub buildrules { # Grab location code from hash. my $loc_src = $$hash{$key}[4];
- # Call function to load the networks list for this country.
&ipset_restore($loc_s rc); + # Check if the network list for this country already has been loaded. + unless($loaded_ipse t_lists{$loc_src}) { + # Call function to load the networks list for this country. + &ipset_rest ore($loc_src);
+ # Store to the hash that this list has been loaded. + $loaded_ips et_lists{$loc_src} = "1"; + }
push(@source_option s, $source); } elsif($source) { @@ -419,8 +426,14 @@ sub buildrules { # Grab location code from hash. my $loc_dst = $$hash{$key}[6];
- # Call function to load the networks list for this country.
&ipset_restore($loc_d st); + # Check if the network list for this country already has been loaded. + unless($loaded_ipse t_lists{$loc_dst}) { + # Call function to load the networks list for this country. + &ipset_rest ore($loc_dst);
+ # Store to the hash that this list has been loaded. + $loaded_ips et_lists{$loc_dst} = "1"; + }
push(@destination_o ptions, $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} = "1"; + }
# Call iptables and create rule to use the loaded ipset list. run("$IPTABLES -A LOCATIONBLOCK -m set -- match-set CC_$location src -j DROP"); -- 2.30.2