New to Perl: Need help with a script

[Jim Moser]

I'm trying to write a simple network monitoring tool in Perl. The
source code is below. The script parses the /etc/hosts file (or a
similarly formated file) and pings each host, giving it a designation
of UP or DOWN. The script repeats this at whatever interval I
designate.

So far the script is functional; however, at each interval I'm calling
system(clear) to clear the screen and update the list. This looks very
sloppy to the end user and I would like the script to keep the IP and
hostname on screen and only update the UP|DOWN field one at a time. If
this is simple to do, could someone provide some sample code? If it's
not quite so simple could someone point me in the right direction to
get started. I am using O'Reilly Programming Perl 3rd Ed and O'Reilly
Perl Cookbook as references.

Also, any conductive criticism is appreciated.

#!/usr/bin/perl

use Net:ing;

# Process arguements
while (@ARGV and $ARGV[0] =~ /^-/) {
$_ = shift;
last if /^--$/;
if (/^-f(.*)/) { $hostfile = $1 }
if (/^-i(.*)/) { $int = $1 }
if (/^-\?|^-h/) { usage(); }
}

sub usage() {
print "Usage: ping.pl options
ping.pl [ -f | -i | -h | -? ]

Options:
-f<filename>

File should be a space delimited file with a list of IP addresses
and hostnames. Use /etc/hosts as an example. If there are multiple
hostnames associated with a single IP address only the first
hostname will be processed. If a file is not specified, the
/etc/hosts file will be used by default.

-i<interval>

This is the interval in seconds to refreshing the server list.
If no interval is specified, a default interval of 30 seconds is
used.

-h display this help screen
-? display this help screen

Examples:
ping.pl -f/etc/hosts -i30\n";
exit;
}

# Set default values if no arguments were supplied
unless (defined($hostfile)) { $hostfile = "/etc/hosts" }
unless (defined($int)) { $int = "30" }

open(HOSTFILE, $hostfile);

# Create a hash of the hostfile; omitting comments, localhost, and
blank lines
LINE: while (<HOSTFILE>) {
next LINE if /^#/;
next LINE if /^127/;
next LINE if /^\s/;
($ip, $hostname) = split /\s/;
@fields = split ' ', $hostname;
$list{$ip} = [ @fields ];
}

# Ping each host once and label UP or DOWN
sub getstatus {
$p = Net:ing->new("icmp");
foreach $server ( keys %list ) {
if ($p->ping($server, 1)) {
$status = "UP";
}else{
$status = "DOWN";
}
write STDOUT;
}
}

# Make it pretty
format STDOUT =
@<<<<<<<<<<<<<<<< @<<<<<<<<<<<<<<<< @<<<<
@{ $list{$server} }, $server, $status
}

while (1) {
system(clear);
getstatus();
sleep($int);
}

 

[Jeff 'japhy' Pinyan]

[posted & mailed]

So far the script is functional; however, at each interval I'm calling
system(clear) to clear the screen and update the list. This looks very
sloppy to the end user and I would like the script to keep the IP and
hostname on screen and only update the UP|DOWN field one at a time. If
this is simple to do, could someone provide some sample code? If it's
not quite so simple could someone point me in the right direction to
get started. I am using O'Reilly Programming Perl 3rd Ed and O'Reilly
Perl Cookbook as references.

If you want to be able to change the text at specific locations on your
terminal, I'd suggest the Curses module.

#!/usr/bin/perl

use Net:ing;

You should always write your code with 'warnings' and 'strict' enabled.
It might take some getting used to (you'll need to declare your variables,
for one thing), but it will help you write cleaner code, and catch any
typos you make, etc.

# Process arguements
while (@ARGV and $ARGV[0] =~ /^-/) {
$_ = shift;
last if /^--$/;
if (/^-f(.*)/) { $hostfile = $1 }
if (/^-i(.*)/) { $int = $1 }
if (/^-\?|^-h/) { usage(); }
}

This is fine, but you should also know that there are standard modules out
there to take care of command-line options for you. See Getopt::Std and
Getopt::Long for starters.

Also, and some others might think me hyper-sensitive for saying this, but
you should really local()ize $_ whenever you assign to it explicitly or
use it when reading from a filehandle, because you can end up clobbering
its value from somewhere else. Consider this example:

my @data = ('a' .. 'e');
for (@data) {
# ...
foo($_)
# ...
}
print "@data"; # no output!

sub foo {
my $file = shift;
open TXT, "< file/$file.txt" or die "can't read file/$file.txt: $!";
while (<TXT>) {
# ...
}
close TXT;
}

@data ends up having 5 undef values in it, because the naked <TXT> syntax
inside a while loop assigns to $_, and $_ is aliased to each of the
elemenst of your for loop list in turn.

sub usage() {

There is no need for the () on your function definition here.

print "Usage: ping.pl options
...
ping.pl -f/etc/hosts -i30\n";
exit;
}

You might want to use a here-doc (perldoc perldata) instead, but it's
really just a preference. More drastically, though, you might consider
documenting your program using POD (perldoc perlpod), and then making your
usage() function:

sub usage {
exec perldoc => __FILE__
}

That runs 'perldoc' on your file, and presents the documentation you've
written in your program.

# Set default values if no arguments were supplied
unless (defined($hostfile)) { $hostfile = "/etc/hosts" }
unless (defined($int)) { $int = "30" }

open(HOSTFILE, $hostfile);

Always, *always*, ALWAYS check the return value of a system call, like an
open():

open HOSTFILE, "< $hostfile" or die "can't read $hostfile: $!";

Also, if you want to be SURE that your file will only be opened for
reading, be explicit about it like I've shown above. If you just do

open FILE, $file;

and you get $file's value from the user, what if they enter

mail (e-mail address removed) < /etc/passwd |

as the filename? Thus, be explicit. (Also look into tainting incoming
data; read perldoc perlsec.)

# Create a hash of the hostfile; omitting comments, localhost, and
blank lines
LINE: while (<HOSTFILE>) {
next LINE if /^#/;
next LINE if /^127/;
next LINE if /^\s/;

That just skips lines that start with any whitespace, not necessarily a
blank line.

($ip, $hostname) = split /\s/;
@fields = split ' ', $hostname;
$list{$ip} = [ @fields ];
}

I would probably rewrite this like so:

while (<HOSTFILE>) {
s/#.*//; # remove any comments
next if /^\s+$/; # skip if it's only whitespace
next if /^127\./; # skip if it's the localhost

my ($ip, @fields) = split;
$list{$ip} = \@fields;
}

You'll notice a couple things different here. First, I'm declaring my
variables with my(). This is because I'd be doing 'use strict', which
requires that I declare my variables' scope. Since I don't need $ip or
@fields to exist outside that while loop, I declare them with my() here.

Also see that instead of doing [ @fields ], I did \@fields. You can't do
this with your code, because your @fields is a global variable, not a
lexical one. If you did \@fields, you'd end up with each hash value
pointing to the same array reference. With my code, because @fields is
lexical and I've taken a reference to it, it doesn't die at the end of the
block, and it lives on (as a reference).

Finally, I've used split() with no arguments. This means the same as
split(' ', $_).

# Ping each host once and label UP or DOWN
sub getstatus {
$p = Net:ing->new("icmp");

Is it *really* necessary to make a new Net:ing object *every* time you
want to ping the hosts? I don't think it is (but I haven't tested it, so
I could be wrong).

foreach $server ( keys %list ) {
if ($p->ping($server, 1)) {
$status = "UP";
}else{
$status = "DOWN";
}
write STDOUT;
}
}

# Make it pretty
format STDOUT =
@<<<<<<<<<<<<<<<< @<<<<<<<<<<<<<<<< @<<<<
@{ $list{$server} }, $server, $status
}

Formats are kinda passe (in my opinion). I'd use printf(), probably.

while (1) {
system(clear);

You should quote that word.

system "clear";

getstatus();
sleep($int);
}

That's all I have to say at first glance.

 

[Jim Moser]

Wow Jeff,

Thanks for all the great input. I appreciate the pointers.

 

[Brian McCauley]

Also, and some others might think me hyper-sensitive for saying this,

Or indeed wrong

but you should really local()ize $_ whenever you assign to it
explicitly or use it when reading from a filehandle, because you can
end up clobbering its value from somewhere else.

The above is commonly given mis-advice.

You should not local()ize $_ using local($_) because you still can end
up clobbering something else if $_ happens to be an alias to an
element of a tied agregate. (This is arguably a bug in Perl.)

Whenever you need to localize $_ then you should do so by saying
local(*_) or for(my $dummy) {}.

If you choose to use local(*_) you need to make sure you've get
everything you want out of the rest of *_ (e.g. @_) first.

--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\