Re: Create a ipset for blocking networks based on internet sources

my $content = $response->content; my @network = split(/\n/,$content); @network = map { m/$regex/ ? $1 : ()} @network; ... foreach (@network) { system("ipset add temp_$set_name $_"); }
[download]

You don't do any error checking on the contents of @network so why not just:

                ...

                foreach ( $response->content =~ /$regex/g ) {
                        system("ipset add temp_$set_name $_");
                        }
[download]

system("ipset create temp_$set_name $set_type"); foreach (@network) { system("ipset add temp_$set_name $_"); } system("ipset create -exist $set_name $set_type"); system("ipset swap temp_$set_name $set_name"); system("ipset destroy temp_$set_name"); my $cron_notice = "IPSet: $set_name updated (as of: $d +ate_now)."; system("logger", "-p", "cron.notice", $cron_notice);
[download]

You don't do any error checking on the system calls. Perhaps something like this:

                0 == system 'ipset', 'create', "temp_$set_name", $set_
+type
                    or warn "Cannot execute ipset because: $?";
[download]

@dates_last = $fh->getlines; @dates_last = map(m/^(\d+).*$/,@dates_last);
[download]

You don't really need two steps there and you don't really need .*$ at the end of the regular expression:

    @dates_last = map /^(\d+)/, $fh->getlines;
[download]

@dates_last = split(/ /,"0 " x $urls_number)
[download]

Or just:

    @dates_last = ( 0 ) x $urls_number
[download]

'(^([0-9]{1,3}\.){3}[0-9]{1,3}).*$', ... '^(\d.*\d).*$', ... '(.*)',
[download]

You should compile your regular expressions here using the qr operator and the .*$ at the end is superfluous.

          qr/(^([0-9]{1,3}\.){3}[0-9]{1,3})/, 
...
        qr/^(\d.*\d)/,
...
        qr/(.*)/,
[download]

@dates_now = map {$_ . "\n"} @dates_now; print $fh @dates_now;
[download]

No need for two steps:

    print $fh map "$_\n", @dates_now;
[download]

Comment on Re: Create a ipset for blocking networks based on internet sources Select or Download Code

Replies are listed 'Best First'.
Re^2: Create a ipset for blocking networks based on internet sources by mimosinnet (Beadle) on Apr 24, 2012 at 18:12 UTC
Jwkrahn, thanks very much for the comments and the links where to find the information. It has been great to have some insight on how to reduce code and to understand the logic behind it. It has also been useful to look at the system command and see how it works with lists (so I now use lists in all the system calls instead of scalars). I have spend some time getting the meaning of ( 0 ) x $ulr_number. Good trick! I have included your suggestions into the original message. Hope this is all right. Very impressed with perl expressiveness: easy to start write code, tricky to master it! For somebody like me without computer knowledge or experience is very exciting! Very greateful! Cheers!	[reply]
Re^3: Create a ipset for blocking networks based on internet sources by jwkrahn (Abbot) on Apr 25, 2012 at 01:49 UTC
You're welcome. I made a little mistake in one sugestion: `qr/(^([0-9]{1,3}\.){3}[0-9]{1,3})/, ... qr/^(\d.\d)/, ... qr/(.)/, ... foreach ( $response->content =~ /$regex/g ) {` [download] And you "corrected" thusly: `qr/\n(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})/, # 46.137.194.0 + 46.137.194.255 24 2650` [download] The problem with that is that that pattern will match every line except the first line. The proper solution is to use the `/m` option so that the pattern will match at the beginning of every line: `qr/(^([0-9]{1,3}\.){3}[0-9]{1,3})/m, ... qr/^(\d.\d)/m, ... qr/(.)/, ... foreach ( $response->content =~ /$regex/g ) {` [download] `my @sys = (qw(ipset create), "temp_$set_name", split / /,$set_ +type); ... @sys = (qw(ipset create -exist), $set_name, split / /,$set_typ +e);` [download] The use of `/ /` with split may not do what you want, and it certainly is not what the shell would do. You should use `' '` instead: `my @sys = (qw(ipset create), "temp_$set_name", split ' ',$set_ +type); ... @sys = (qw(ipset create -exist), $set_name, split ' ',$set_typ +e);` [download] `$fh->open("> $f_dates_last") \|\| die "Unable to save timestamp urls in +$f_dates_last: $?";` [download] The $? variable will have no useful information if open fails. You should use $! or $^E instead.	[reply] [d/l] [select]
Re^4: Create a ipset for blocking networks based on internet sources by mimosinnet (Beadle) on Apr 25, 2012 at 18:40 UTC
Thanks jwkrahn, I really appreciate the corrections! I have included them in the script and read the section on error variables in perlvar. Also, my first version used Moose (influenced by the book "Modern Perl"). After looking at the script, and comparing it with the bash scripts, it took more time and resources. I guess that Moose is for larger and more complex projects, so I have rewritten in what I assume is "procedural programming". It has been trivial to have Moose out, transforming the package into a subroutine. One thing is puzzeling me is the regex. In a file in this format: `Start End Netblock Attacks Name Country email 116.45.99.0 116.45.99.255 24 1799 46.21.150.0 46.21.150.255 24 1708 121.243.146.0 121.243.146.255 24 1446` [download] Applying this regex: `qr/^(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})/m` Gives me: `116.45.99.0 46.21.150.0 121.243.146.0` [download] And this regex: `qr/^((\d{1,3}\.){3}\d{1,3})/m` Gimes me: `116.45.99.0 99. 46.21.150.0 150. 121.243.146.0 146.` [download] I would expect the same result.	[reply] [d/l] [select]
Re^5: Create a ipset for blocking networks based on internet sources by jwkrahn (Abbot) on Apr 25, 2012 at 19:10 UTC
Re^6: Create a ipset for blocking networks based on internet sources by mimosinnet (Beadle) on Apr 27, 2012 at 17:23 UTC