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}\"
> [email protected] < $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: [email protected]
For additional commands, e-mail: [email protected]
http://learn.perl.org/
