while loop into database insert not working

[Unknown]

Hello All,

I am working on this script which will parse several Backup Exec logs
and place key details into a database for viewing as statistics
on a web based front end. I posted all the code, but I am only having
trouble with the while loop (inside the foreach). It is not behaving the
way I expect it to in that it prints several copies of each record
instea of just one record for one text file.

thanks
Rocky



#!/usr/bin/perl
use warnings;
use strict;
use DBI;
use DBD:g;

my $filedir = '/var/backuplogs';
my $servername;
my $jobname;
my $jobstarted;
my $medialabel;
my $logfile;
my $jobended;
my $status;
my @inuse;
my @skipped;
my @corrupt;
my @skippednames;
my $skipped;
my $corrupt;
my $inuse;


opendir(DIR, $filedir) or die "could not open logfile directory: $!";
my @filestoparse = grep { /\.txt$/ } readdir(DIR);
closedir(DIR);

foreach my $filename (@filestoparse) {
my $complete = "$filedir/" . "$filename";
open(FH, "$complete") or die "Cannot find file $complete: $!";
while (<FH>) {
if ($_ =~ /^Job\sserver:\s+(.*)$/) { $servername = $1;}
if ($_ =~ /^Job\sname:\s+(.*)$/) { $jobname = $1;}
if ($_ =~ /^Job\sstarted:\s+(.*)$/) { $jobstarted = $1;}
if ($_ =~ /^Media.*Label:\s+(.*)$/) { $medialabel = $1;}
if ($_ =~ /^Job\sended:\s+(.*)$/) { $jobended = $1;}
if ($_ =~ /^Job\scompletion\sstatus:\s+(.*)$/) { $status = $1;}
if ($_ =~ /^(\d+)\s+files\swere\sin.*$/) {push(@inuse, $1);}
if ($_ =~ /^(\d+)\s+items/) {push(@skipped, $1);}
if ($_ =~ /^(\d+)\s+corrupt/) {push(@corrupt, $1);}
if ($_ =~ /^.*item(.*\s-\sskipped.*)$/) {push(@skippednames, $1);}

$corrupt = &addnums(@corrupt);
$skipped = &addnums(@skipped);
$inuse = &addnums(@inuse);
$logfile = $filename;

}
close(FH);
print "$jobname,$jobstarted,$logfile,$servername\n";

my $dbh = DBI->connect( "dbig:dbname=backup", "[snip]", "[snip]") or die
"Cannot connect to PGSQL DB\n"; my $sth = $dbh->prepare("INSERT INTO
backuplogs(jobname,jobstarted,logfile,servername,status,inuse,jobended,skipped,corrupt,medialabel,testlogfile)
VALUES
('$jobname','$jobstarted','$logfile','$servername','$status','$inuse','$jobended','$skipped','$corrupt','$medialabel','<a
href=http://[snip]/oldlogs/$logfile>$logfile</a>')")
or die "Error preparing row: $DBI::errstr\n"; $sth->execute() or die "
$DBI::errstr\n"; $dbh->disconnect;
$servername = "";
$jobname = "";
$jobstarted = "";
$medialabel = "";
$jobended = "";
$status = "";
}


sub addnums {
my @number = @_;
my $total;
my $digit;
foreach $digit (@number) {
$total += $digit;
}

if (defined($total)) {
$total = $total;
}
else
{$total = 0;}
return $total;
}

 

[axel]

I am working on this script which will parse several Backup Exec logs
and place key details into a database for viewing as statistics
on a web based front end. I posted all the code, but I am only having
trouble with the while loop (inside the foreach). It is not behaving the
way I expect it to in that it prints several copies of each record
instea of just one record for one text file.

I can see nothing obviously wrong with it... although the while loop has
no print statements - the only print statement is within the foreach
loop.

foreach my $filename (@filestoparse) {
my $complete = "$filedir/" . "$filename";
open(FH, "$complete") or die "Cannot find file $complete: $!";
while (<FH>) {
[snip]
}
close(FH);
print "$jobname,$jobstarted,$logfile,$servername\n";

In such cases I would place a few diagnostic print statements to make
sure that your data is what you are expecting - e.g. the list of files
to be opened and so on.

Axel

 

[David K. Wall]

I am working on this script which will parse several Backup Exec
logs and place key details into a database for viewing as
statistics on a web based front end. I posted all the code, but
I am only having trouble with the while loop (inside the
foreach). It is not behaving the way I expect it to in that it
prints several copies of each record instea of just one record
for one text file.

I can't tell what might be causing that behaviour, but I do have a
few comments that I hope will be helpful.

By the way, I do hope your code is formatted better than it appears
in this post. Proper indentation makes code much easier to read
and debug -- and makes it more likely that someone in this newsgroup
will bother to try and help.

#!/usr/bin/perl
use warnings;
use strict;
use DBI;
use DBD:g;

my $filedir = '/var/backuplogs';
my $servername;
my $jobname;
my $jobstarted;
my $medialabel;
my $logfile;
my $jobended;
my $status;
my @inuse;
my @skipped;
my @corrupt;
my @skippednames;
my $skipped;
my $corrupt;
my $inuse;

Most of these variables are only used inside the foreach loop, so
it's best to declare them at the top of the loop, confining their
scope to ONLY the foreach loop. That way you don't have to worry
about re-initializing them at the end of the loop. (Maybe this is
the problem?)

opendir(DIR, $filedir) or die "could not open logfile directory: $!";
my @filestoparse = grep { /\.txt$/ } readdir(DIR);
closedir(DIR);

foreach my $filename (@filestoparse) {

Why not just say

foreach my $logname (@filestoparse) {

Then you would have one fewer variable to keep track of.

my $complete = "$filedir/" . "$filename";

This is slightly odd. The double quotes around $filename are
unnecessary. See 'perldoc -q quoting'. If you really insist
on using the string concatenation operator, I'd probably
write it as

my $complete = $filedir . '/' . $filename;

but it's probably easiest to read and write as

my $complete = "$filedir/$filename";

Or, since I recommended getting rid of an extra variable,

my $complete = "$filedir/$logname";

open(FH, "$complete") or die "Cannot find file $complete: $!";

The parentheses aren't really necessary when you use the
low-precedence or (instead of ||).

while (<FH>) {
if ($_ =~ /^Job\sserver:\s+(.*)$/) { $servername = $1;}

You can shorten this. Pattern matches automatically look at $_
if you don't specify a string.

if ( /^Job\sserver:\s+(.*)$/ ) { $servername = $1;}

if ($_ =~ /^Job\sname:\s+(.*)$/) { $jobname = $1;}
if ($_ =~ /^Job\sstarted:\s+(.*)$/) { $jobstarted = $1;}
if ($_ =~ /^Media.*Label:\s+(.*)$/) { $medialabel = $1;}
if ($_ =~ /^Job\sended:\s+(.*)$/) { $jobended = $1;}
if ($_ =~ /^Job\scompletion\sstatus:\s+(.*)$/) { $status = $1;}
if ($_ =~ /^(\d+)\s+files\swere\sin.*$/) {push(@inuse, $1);}

Later on you just add up the numbers collected in @inuse, so why not
eliminate the array and add them up as you go?

if ( /^(\d+)\s+files\swere\sin.*$/) { $inuse += $1 }

if ($_ =~ /^(\d+)\s+items/) {push(@skipped, $1);}
Ditto....

if ($_ =~ /^(\d+)\s+corrupt/) {push(@corrupt, $1);}
if ($_ =~ /^.*item(.*\s-\sskipped.*)$/) {push(@skippednames, $1);}

I'm not sure why this line is even here. You don't use @skippednames
anywhere else, so why bother to collect it?

$corrupt = &addnums(@corrupt);

No need for the & on the call to addnums(). If you don't need the effect
that the & gives you, don't bother with it. See 'perldoc perlsub'.

$skipped = &addnums(@skipped);
$inuse = &addnums(@inuse);
$logfile = $filename;

}
close(FH);
print "$jobname,$jobstarted,$logfile,$servername\n";

my $dbh = DBI->connect( "dbig:dbname=backup", "[snip]", "[snip]") or die
"Cannot connect to PGSQL DB\n";

I'd put this outside the foreach loop. Connect ONCE, then insert new
rows as necessary.

my $sth = $dbh->prepare("INSERT INTO
backuplogs(jobname,jobstarted,logfile,servername,status,inuse,jobended,skipped,corrupt,medialabel,testlogfile)
VALUES
('$jobname','$jobstarted','$logfile','$servername','$status','$inuse','$jobended','$skipped','$corrupt','$medialabel','<a
href=http://[snip]/oldlogs/$logfile>$logfile</a>')")
or die "Error preparing row: $DBI::errstr\n"; $sth->execute() or die "
$DBI::errstr\n";
$dbh->disconnect;

And I'd move the disconnect to be after the foreach loop.

$servername = "";
$jobname = "";
$jobstarted = "";
$medialabel = "";
$jobended = "";
$status = "";

These re-initializations can be dropped when you confine the variables
to the smallest possible scope (as mentioned above).

}


sub addnums {
my @number = @_;
my $total;
my $digit;
foreach $digit (@number) {
$total += $digit;
}

if (defined($total)) {
$total = $total;
}
else
{$total = 0;}
return $total;
}

This sub becomes unnecessary if you accumulate the sum as you go. But
even so, it could be shorter (but not *too* short, I hope).

sub addnums {
return 0 unless @_;
my $total = 0;
local $_; # don't step on $_
$total += $_ for @_;
return $total;
}

 

[nobull]

sub addnums {
return 0 unless @_;

Don't create unecessary special cases

my $total = 0;
local $_; # don't step on $_

The for statement qualifier will localize $_ properly anyhow.

local($_) on the other hand, will do bad things if $_ happens to be an
alias for an element of a tied agregate.

$total += $_ for @_;
return $total;
}

sub addnums {
my $total = 0;
$total += $_ for @_;
return $total;
}

 

[Fabian Pilkowski]

my $filedir = '/var/backuplogs';

[...]

my $inuse;

Most of these variables are only used inside the foreach loop, so
it's best to declare them at the top of the loop, confining their
scope to ONLY the foreach loop. That way you don't have to worry
about re-initializing them at the end of the loop. (Maybe this is
the problem?)

Additionally, I propose to use a hash for this. Then you have not to
declare all those vars and you can simplify the sql statement below.

Why not just say

foreach my $logname (@filestoparse) {
my $complete = "$filedir/$logname";

You can do all of this in one step by using glob(). But then you lose
the var $logname also. If necessary, determine it afterwards, e.g.

foreach my $complete ( glob "$filedir/*.txt" ) {
my $hash;
( $hash{logfile} ) = $complete =~ m#([^/]+)$#;

You can shorten this. Pattern matches automatically look at $_
if you don't specify a string.

if ( /^Job\sserver:\s+(.*)$/ ) { $servername = $1;}


Later on you just add up the numbers collected in @inuse, so why not
eliminate the array and add them up as you go?

if ( /^(\d+)\s+files\swere\sin.*$/) { $inuse += $1 }

Here, I prefer to put the condition behind the assignment: This saves
many parentheses and provides more readability.

$hash{servername} = $1 if /^Job\sserver:\s+(.*)$/;
$hash{jobname} = $1 if /^Job\sname:\s+(.*)$/;
$hash{jobstarted} = $1 if /^Job\sstarted:\s+(.*)$/;
$hash{medialabel} = $1 if /^Media.*Label:\s+(.*)$/;
$hash{jobended} = $1 if /^Job\sended:\s+(.*)$/;
$hash{status} = $1 if /^Job\scompletion\sstatus:\s+(.*)$/;

$hash{inuse} += $1 if /^(\d+)\s+files\swere\sin.*$/;
$hash{skipped} += $1 if /^(\d+)\s+items/;
$hash{corrupt} += $1 if /^(\d+)\s+corrupt/;

}

my $sth = $dbh->prepare("INSERT INTO
backuplogs(jobname,jobstarted,logfile,servername,status,inuse,jobended,skipped,corrupt,medialabel,testlogfile)
VALUES
('$jobname','$jobstarted','$logfile','$servername','$status','$inuse','$jobended','$skipped','$corrupt','$medialabel','<a
href=http://[snip]/oldlogs/$logfile>$logfile</a>')")
or die "Error preparing row: $DBI::errstr\n"; $sth->execute() or die "$DBI::errstr\n";

Since all needed values are gathered in the new hash ('testlogfile'
could be added separately), we could build up this statement a little
bit easier, I think.

my $keys = join ',', keys %hash;
my $bind = join ',', ('?') x keys %hash;
my $sth = $dbh->prepare( "INSERT INTO backuplogs ($keys) VALUES ($bind)" )
or die "Error preparing row: $DBI::errstr\n";
$sth->execute( values %hash ) or die "$DBI::errstr\n";

This will work because keys() and values() return their lists for one
hash in the same order. And using binding vars in the statement (»?«)
will be less fragile than that *ugly* self-made-quoting.

These re-initializations can be dropped when you confine the variables
to the smallest possible scope (as mentioned above).

But the arrays @inuse, @skipped and @corrupt keep their values, there is
no reset for them. As a result, the sums $inuse etc contain the sum of
all numbers ever found. If this is really wanted? I don't know. But due
to using a hash, we could delete some vars explicitly. Perhaps by

my @keys = qw( servername jobname jobstarted medialabel jobended status );
@hash{ @keys } = ('') x @keys;

Otherwise its easier to declare the hash inside of the for loop.

This sub becomes unnecessary if you accumulate the sum as you go. But
even so, it could be shorter (but not *too* short, I hope).

sub addnums {
return 0 unless @_;

You can throw away this line as well if you want to shorten this sub. If
@_ is *false* the loop below won't entered, so $totel remains at zero.
And returning zero is nothing else you're doing here ;-)

my $total = 0;
local $_; # don't step on $_
$total += $_ for @_;
return $total;
}

We do not need this sub, but don't "don't step on $_" here. A for loop
is localizing $_ implicitly. Try out:

$_ = 'sum = ';
my $total = 0;
$total += $_ for 1, 2, 3;
print $_, $total;
__END__
sum = 6

regards,
fabian


Postscript: Finally, I want to summarize the code -- just for clarity.

#!/usr/bin/perl
use warnings;
use strict;
use DBI;
use DBD:g;

my $filedir = '/var/backuplogs';
my %hash = ();
my $dbh = DBI->connect( "dbig:dbname=backup", "[snip]", "[snip]")
or die "Cannot connect to PGSQL DB\n";

foreach my $file ( glob "$filedir/*.txt" ) {

# read in the file
open FH, '<', $file or die "Cannot open file '$file': $!";
while ( <FH> ) {
$hash{servername} = $1 if /^Job\sserver:\s+(.*)$/;
$hash{jobname} = $1 if /^Job\sname:\s+(.*)$/;
$hash{jobstarted} = $1 if /^Job\sstarted:\s+(.*)$/;
$hash{medialabel} = $1 if /^Media.*Label:\s+(.*)$/;
$hash{jobended} = $1 if /^Job\sended:\s+(.*)$/;
$hash{status} = $1 if /^Job\scompletion\sstatus:\s+(.*)$/;

$hash{inuse} += $1 if /^(\d+)\s+files\swere\sin.*$/;
$hash{skipped} += $1 if /^(\d+)\s+items/;
$hash{corrupt} += $1 if /^(\d+)\s+corrupt/;
}
close FH;
( $hash{logfile} ) = $file =~ m#([^/]+)$#;
$hash{testlogfile} = "<a href=http://[snip]/oldlogs/$hash{logfile}>$hash{logfile}</a>";

# send it to db
my $keys = join ',', keys %hash;
my $bind = join ',', ('?') x keys %hash;
my $sth = $dbh->prepare( "INSERT INTO backuplogs ($keys) VALUES ($bind)" )
or die "Error preparing row: $DBI::errstr\n";
$sth->execute( values %hash ) or die "$DBI::errstr\n";

# reset some values
my @keys = qw( servername jobname jobstarted medialabel jobended status );
@hash{ @keys } = ('') x @keys;
}

$dbh->disconnect;
__END__

 

[David K. Wall]

The for statement qualifier will localize $_ properly anyhow.

local($_) on the other hand, will do bad things if $_ happens to
be an alias for an element of a tied agregate.

I don't often have a need for tied variables in the little utility
programs I mostly write, so I don't really see the problem. Could you
elaborate on this?

 

[Anno Siegel]

Unknown <[email protected]> wrote:

[good advice]

This sub becomes unnecessary if you accumulate the sum as you go. But
even so, it could be shorter (but not *too* short, I hope).

It's not short enough yet

sub addnums {
return 0 unless @_;

Not necessary if you have the next line.

my $total = 0;

Not necessary if you have the previous line.

local $_; # don't step on $_

Not necessary. A for-loop localizes $/ for you.

$total += $_ for @_;
return $total;
}

So:

sub addnums {
my $total = 0;
$total += $_ for @_;
$total;
}

or

use List::Util;
*addnums = \ &List::Util::sum;

Anno

 

[David K. Wall]

I don't often have a need for tied variables in the little utility
programs I mostly write, so I don't really see the problem. Could
you elaborate on this?

[sound of crickets]

As usual, an echoing silence means I should have checked the docs.

In perltie, the Bugs section says this:

"Localizing tied arrays or hashes does not work. After exiting the
scope the arrays or the hashes are not restored."

I suppose I could read the code for the relevant modules to find out
exactly why, but I'm not /that/ interested.