On Sep 28, FamiLink Admin said:
I am trying to read a log file and get a list of how many times an IP
address get blocked each hour by category PO. An example line in the log
with a block is:
-------------
[2005-09-28 10:05:03 -7:00] 127.0.0.1 71.32.59.249 216.163.137.3 -
http://www.playboy.com/ blocked 0 PO
-------------
What I have kinda works but I am not sure if it is the best practice.
This is the first time programming in perl and this is what I have so
far:
Your indentation leaves much to be desired, so I've "fixed" it.
sub Scanlog {
local($ipb) = @_;
No reason to use 'local'; stick with 'my' here. But... what is $ipb? You
don't use it anywhere!
open my $slog, "-|", "tail -n 50000 $log" or die "Unable to open $log:$!\n";
open (OUTPUT,">/etc/squid/iplist.txt");
open (OUTPUT2,">/etc/squid/SuspendIpList.txt");
You should also die if neither of those could be opened:
open(OUTPUT, ">...") or die "can't create /etc/squid/iplist.txt: $!";
while (<$slog>){ # assigns each line in turn to $_
# use an array slice to select the fields we want
@data = (split ,$_)[1,4,10,5,7];
$hr = (split /:/ ,$data[0])[0];
$ip = "$data[1]";
Those three variables should all be declared with 'my'. Your line
assigning to @data has a typo that hasn't effected you, but it might
eventually.
my @data = (split)[1,4,10,5,7]; # why out of order?
my $hr = (split /:/, $data[0])[0];
my $ip = $data[1]; # no need to quote $data[1] here
if ($flag eq $data[2]) {
Where is $flag coming from?
if ($hr eq $hour) {
Where is $hour coming from?
Those two if statements can be combined into one, since you don't do
anything if they aren't both true.
if ($flag eq $data[2] and $hr eq $hour) {
foreach (/$data[2]/) {
$matches += 1 ;
}
I have a feeling this could lead to false positives. How do you know that
'PO' (or whatever else $data[2] might hold) won't appear in the URL, for
instance? Perhaps this should just be
$matches++;
But where is $matches coming from?!
if ($matches > $blocklimit) {
Where does $blocklimit come from?!
$ip1 = "$data[1]/32";
Declare that with 'my'.
print OUTPUT "$matches,", "$hour, ","$ip1, ", "@data","\n";
You could just write that as
print OUTPUT "$matches, $hour, $data[1]/32 @data\n";
print OUTPUT2 "$ip1\n";
$matched = $matches;
$matches = 0;
Where did $matched come from?
}
}
}
}
close (OUTPUT);
close (OUTPUT2);
}
You should not use any variables in a function that you did not pass to it
or create IN it.
--
Jeff "japhy" Pinyan % How can we ever be the sold short or
RPI Acacia Brother #734 % the cheated, we who for every service
http://www.perlmonks.org/ % have long ago been overpaid?
http://princeton.pm.org/ % -- Meister Eckhart
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
<http://learn.perl.org/> <http://learn.perl.org/first-response>
