Re: [PATCH 09/12] rules.pl: Do not try to restore the same ipset multiple times.

[Stefan Schantl <stefan.schantl@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 6929 bytes --]

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(a)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(a)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
> > 
>