Re: Debug Help Please



Andy wrote:
Greets

Hello,

I wrote this script to parse information from some log files. It Seems
to work , or look like it works.
In the end I get the log creation of .csv log files with no
information.

I am learning how to write this , I know there are tons of ways to do
this. But My Boss made a decision to keep all the scripts looking
similar, so I have to keep the format the same.

What did I do wrong that there is no information in the log files?

#!/usr/bin/perl

The next two lines *should* be:

use warnings;
use strict;

They will *help* you find errors in your code.


#Define LogFiles
my $dateroot="$ARGV[0]"; # Value should be 2-digit month

perldoc -q "What.s wrong with always quoting .$vars.?"


my $dateroot = $ARGV[ 0 ]; # Value should be 2-digit month


my $outgoing="outgoing_xferlog.$dateroot.csv"; # This is for all
files sent to users
my $incoming="incoming_xferlog.$dateroot.csv"; #This is log for
uploads
my $loginsinlog="loginsinlog_xferlog.$dateroot.csv"; # This is where
log users
my $completedlog="completedlog_xferlog.$dateroot.csv"; # All
Completed Sessions
my $failedlog="failedlog_xferlog.$dateroot.csv"; # This is for
all Failures
my %loginsin;
my %completedlog;
my %failures;
my %fields;

%fields is never used so why are you creating it?

You also need to declare the %incoming and %outgoing hashes:

my %incoming;
my %outgoing;


#Time Measured
print "Started Processing Logfiles for $dateroot at " . (localtime
time) ."\n\n";
#Open Log File Dir to Read .log file(s)
opendir (DIR, ".") or die "$!";

my @logfiles = grep {/xferlog\.$dateroot.*/} readdir (DIR);

The .* at the end is superfluous. The {} braces create a scope that you don't need.

my @logfiles = grep /xferlog\.$dateroot/, readdir DIR;


close DIR;

#Start Log Processing
foreach my $logfile (@logfile) {

The variable @logfile is empty, it should be @logfiles instead, which is why you get an empty output file. If you had enabled the strict pragma then perl would have notified you of this mistake.


print "Started Processing: $dateroot at " . (localtime time) ."\n";
open(FH,"./$logfile") or die "$!";
while (<FH>){
my $line = $_;

Why not just:

while ( my $line = <FH> ) {

Or why not just use the $_ variable instead?


chomp($line);
my @fields = split / /, $line; #This is where we look

Did you really *really* *REALLY* mean to split on a single space character? Or do you not really understand how split works?


for the . extensions from the file name

#My Ftp Status Log Values
foreach ($line) {

OMG *why* do you have a foreach loop here?


my $TIME =$fields[0],[1],[2],[3];

That is the same as:

my $TIME = [3];

Which assigns an anonymous array reference to the variable $TIME with one element with the value of 3.

If you had enabled the warnings and strict pragmas then perl would have informed you of this mistake.



my $Year = $fields[4];

The variable $Year is not used anywhere else, perhaps you meant $YEAR instead.


my $TRANSFER= $fields[5];#this value is in seconds

The variable $TRANSFER is never used anywhere else.


my $IP = $fields[6];
my $SIZE = $fields[7]; #filesize
my $FILE = $fields[8]; #filename and path
my $TYPE = $fields[9]; #A= ascii B = binary

The variable $TYPE is never used anywhere else.


my $DIRECTION = $fields[11]; #Outgoing, Incoming
my $USERNAME = $fields[13];
my $STATUS= $fields[17]; #c = completed i=
incomplete
my $MASKFILE = $FILE;
my $MASKFILE =~ s/\d/#/g;

my creates an empty variable so that line makes no sense. If you had enabled the warnings and strict pragmas perl would have notified you of this mistake. You probably should have written:

( my $MASKFILE = $FILE ) =~ tr/0-9/#/;


#Failures This is where we check for
failures
if ($STATUS eq "i" ){$failures {$USERNAME} = $TIME.",".$YEAR.",".
$FILE.",".$IP;}
#completed sessions Last Login
if ($STATUS eq "c" ){$completedlog {$USERNAME} = $TIME.",".$YEAR.",".
$IP;}

#Completed incoming
if ($DIRECTION eq "i" and $STATUS eq "c" ){$incoming {$USERNAME} =
$TIME.",".$YEAR.",".$FILE.",".$SIZE.",".$IP;}
#Outgoing this is where we log all
outgoing xfers
if ($DIRECTION eq "o" and $STATUS eq "c" ){$outgoing {$USERNAME} =
$TIME.",".$YEAR.",".$FILE.",".$SIZE.",".$IP;}

#Masked Options with file
extensions WIll be added for later use
#if ($DIRECTION eq "i" and $STATUS eq "c" ){$completedlog
{$USERNAME.",".$MASKFILE} = $FILE.",".$TIME.",".$YEAR.",".$IP.",".
$STATUS;}
}
next;

next at the end of the loop is superfluous. Where else is the loop going to go?


}
close(FH);
}
open(OUTPUT, '>', $outgoing) or die("Could not open log file.");
for my $key ( sort %logins) { if ($logins{$key}) { print OUTPUT "$key,

The hash %logins is never populated with any data so this loop will be bypassed.


$logins{$key}\n";}}
close(OUTPUT);
open(OUTPUT, '>', $incoming) or die("Could not open log file.");
for my $key ( sort %failures) { if ($failures{$key}) { print OUTPUT

You are sorting the keys *and* values of the hash. That should be:

for my $key ( sort keys %failures ) {


"$key,$failures{$key}\n";}}
close(OUTPUT);
open(OUTPUT, '>', $loginsinlog) or die("Could not open log file.");
for my $key ( sort %loginsin) { if ($loginsin{$key}) { print OUTPUT

The hash %loginsin is never populated with any data so this loop will be bypassed.


"$key,$loginsin{$key}\n";}}
close(OUTPUT);
open(OUTPUT, '>', $failedlog) or die("Could not open log file.");
for my $key ( sort %failures) { if ($failures{$key}) { print OUTPUT

You are sorting the keys *and* values of the hash. That should be:

for my $key ( sort keys %failures ) {


"$key,$failures{$key}\n";}}
open(OUTPUT, '>', $completedlog) or die("Could not open log file.");
for my $key ( sort %failures) { if ($failures{$key}) { print OUTPUT

You are sorting the keys *and* values of the hash. That should be:

for my $key ( sort keys %failures ) {


Why are you looping through %failures *three* *different* *times*? Just use one loop and output to all three files at once.


"$key,$failures{$key}\n";}}
close(OUTPUT);
print "\nFinished Processing Logfiles for $dateroot at " . (localtime
time) ."\n\n";
print "This script took ". (time - $^T)/60 ." minutes \n"


John
--
Perl isn't a toolbox, but a small machine shop where you
can special-order certain sorts of tools at low cost and
in short order. -- Larry Wall
.