On Wed, May 5, 2010 at 10:18 AM, Shlomi Fish <shlo...@iglu.org.il> wrote:
> Hi Paul,
>
> a few comments on your code - so you'll know how to write Perl better.
>
> On Wednesday 05 May 2010 02:52:03 Paul Fontenot wrote:
> > Hi,
> >
> > I'm stuck on using an array to determine the out come of a foreach loop.
> > The script is below.
> >
> ---------------------------------------------------------------------------
> > ---------------------------------
> > #!/usr/bin/perl
> > #
>
> Always add "use strict;" and "use warnings;" at the top of your programs
> (and
> fix whatever they point to).
>
> > @conditions = ("NET", "eth");
>
> Declare your variables using "my". (use strict requires it).
>
> > $hostname = (`/bin/hostname`);
>
> Why are the backticks inside a "( ... )"? It shouldn't hurt, though.
>
> > $logfile = "/var/log/messages";
> > $output = "/root/bin/output.txt";
> >
> > open(OUTPUT, ">>", $output) or die $!;
> > open(LOGFILE, "<", $logfile) or die $!;
> >
>
> It's good that you're using three-args-open followed by "or die", but:
>
> 1. You should include a more descriptive error.
>
> 2. Don't use bareword filehandles - use lexical ones:
>
> open(my $output, ">>", $output) or die "Could not append to output - $!";
>
> > foreach $condition (@conditions) {
> > print "\n";
> > print "$condition detected on $hostname";
> > print
> >
> "=========================================================================\
> > n";
>
> I don't understand why you are printing this message without checking for
> this
> condition?
>
> >
> > foreach $line (<LOGFILE>) {
>
> while (my $line = <$logfile>) would be a better idea than foreach $line.
>
> Furthermore, this won't work at the second iteration of the @conditions
> loop
> because LOGFILE reached end-of-file (EOF). You probably need to open it in
> every iteration of the loop.
>
> > if ($line =~ /$condition/) {
>
> 1. Perhaps you want to check for both conditions one after the other, and
> collect their results, so you won't have to go over the file's lines twice.
>
> 2. Doing /$condition/ will evaluate "$condition" as a mini-regex, which is
> potentially dangerous. Please consider doing \Q$condition\E or maybe use
> http://perldoc.perl.org/functions/index.html .
>
> > print $line;
> > }
> > }
> > }
> >
> > close(OUTPUT);
> >
> > if ( -s $output) {
> > system(`/bin/mail -s \"$condition for \${HOSTNAME}\"
> > wpfonten...@cox.net < $output`);
>
> You shouldn't use both system and backicks. Which one of them do you need?
>
> > unlink $output;
> > }
> > else {
> > unlink $output;
> > }
>
> You should probably put the unlink after the if/else conditional to avoid
> duplicate code.
>
> > close(LOGFILE);
> >
>
> Regards,
>
> Shlomi Fish
>
> --
> -----------------------------------------------------------------
> Shlomi Fish http://www.shlomifish.org/
> Parody on "The Fountainhead" - http://shlom.in/towtf
>
> God considered inflicting XSLT as the tenth plague of Egypt, but then
> decided against it because he thought it would be too evil.
>
> Please reply to list if it's a mailing list post - http://shlom.in/reply .
>
> --
> To unsubscribe, e-mail: beginners-unsubscr...@perl.org
> For additional commands, e-mail: beginners-h...@perl.org
> http://learn.perl.org/
>
>
>
Would it not be more efficient to reset the file handle to the star of the
file?
use strict;
use warnings;
use Fcntl qw(:seek);
...
foreach my $condition (@conditions) {
seek ( $fh, 0, 0 ) or die "ERROR: Could not reset file handle\n";
while (<LOGFILE>) {
my $line = $_;
...
}
}
That way there is not need to re-open the file handle over and over again
saving on that IO overhead. Of course you need to then deal with the
posibility of the file being rotated etc. if this is a long running script,
but if this is a once every now and then started script say once every 5 or
10 minutes then you should be fine doing this.
If you want this script to run all the time in a never ending loop and deal
with rotating files due to size/dates etc, then the reopening of the file is
the simplest way of doing this.
Regards,
Rob
