in reply to using -T doesn't work

using -T doesn't work
At a first glance I don't see anything wrong with your code that may involve -T. But then I must admit that I don't have much experience with CGI programming. However your code at certain points is rather clumsy and hard to follow. Of course I'm the first one to think that this is largely a matter of personal tastes, but in any case I'll try to make a few cmts that hopefully you may find to be useful.

Before proceeding, however, I can't help pointing out that your description of the problem is particularly poor: generally "doesn't work" doesn't give any clue on what may actually be wrong. So the first suspect one can legitimately have is that your problem does not really have to do with -T. In particular I've noticed that you misplaced the header, so that may bring you to a "doesn't work" situation. But due to the precise nature of your question and willing to trust you, I assume that you have tried both with -T and without. Well, in any case this is not a good reason not to use it. Adopt the standard method to untaint any tainted variable you want to, instead, if really needed!

Let's see the code, anyway... 

use strict;
use warnings;

[download]
Good! 
use Net::Ping;
use CGI qw/:standard/;

[download]
Also good. 
my @hosts;
if ( -e "ship.cfg" ) {
        open (CFGFILE, "ship.cfg") || die;
        @hosts=<CFGFILE>;
        close (CFGFILE);
} else {
        # Enter your hosts here...
        @hosts = qw/host1 host2 host3/;
}

[download]
Well, when die()ing upon a failed system call it's generally advised to include relevant info. A minimal C<die $!> can be a good start...

As a side note, you chomp() values below, and it obviously won't affect the latter entries that do not have an ending "\n", so it is indeed consistent. But IMHO it is slightly stylistically inconsistent: if it were me I would prefer to have all the values chomp()ed at this point.

UPDATE: this is really a minor point, but you rather than having two literal "ship.cfg" it may be better to assign to a variable to reduce the risk of typos. You may even want to 

for ('ship.cfg') {
    # ...
}

[download]

UPDATE2: it didn't occur to me first because I naively and mistakenly thought that since you hardcoded the config file in the source, anything coming from it would not have been tainted. But that's still data coming in from the outside so indeed it is tainted! As I wrote above, just untaint it! 

my @uphosts=();
my @downhosts=();

[download]
No need for C<()>. 
my $proto;
my @verbage=("are", "hosts", "are", "hosts");

[download]
Are you really using C<$proto>? Also, you're only moderately doing this now, but it's better to avoid altogether to predeclare all needed variables in one point. It's much better to declare them when they are used instead.

You may want to know that you can declare more than one variable at a time, e.g.: my (@uphosts, @downhosts).

UPDATE: this is another minor point, but since you used qw// above I wonder why you didn't use it here too, since it makes for a much more agile syntax. 

my $p = Net::Ping->new('udp');

foreach my $host(@hosts) {
        chomp($host);
        if ($p->ping($host)) {
                push (@uphosts, $host);
        } else {
                push (@downhosts, $host);
        }
}

[download]
So far so fine, although I'd probably do it in a different manner. Only I'd like to add that while I use myself the C<for my $var (@list)> constructs for clarity, with such small blocks I prefer to use the topicalizer. 
$p->close();
print start_html,br;
print header,br;
print start_form,br;
print submit('Refresh'),p;

[download]
Huh?!? This is not a Perl issue, but do you really want to print C<header> after C<start_html>?!? 
if (scalar(@downhosts) == 1 ) {
        $verbage[2]="is";
        $verbage[3]="host"
}
if (scalar(@uphosts) == 1 ) {
        $verbage[0]="is";
        $verbage[1]="host";
}

[download]
No need for scalar().

You can use an array slice. 

unless (scalar(@downhosts) == 0) { print p,"There $verbage[2] ",scalar

[download]
No need for the first scalar. 
(@downhosts),"  $verbage[3] down",br }
print "There $verbage[0] ",scalar(@uphosts)," $verbage[1] alive",br;

print p,"The following $verbage[1] $verbage[0] alive: \n",br;
foreach my $uitem (sort @uphosts) {
        print li("$uitem"),br;
}

[download]
Ditto as above wrt the topicalizer: you can also use C<for> as a statement modifier.

UPDATE: please note that there's non need to quote all variables, see 'perldoc -q quoting'. 

unless (scalar(@downhosts) == 0) { print p,"The following $verbage[3] 
$verbage[2] down: ",br }
foreach my $ditem (sort @downhosts) {
        print li("$ditem"),br;
}

[download]
Ditto as above wrt scalar().

Ditto as above wrt the topicalizer.

All in all I'd rewrite the code as follows:

The full code, revisited

Note: I'm not saying that this is the best way to do it, it's just one of the many possible ones. Be warned too that it's untested... 
#!/usr/bin/perl

use strict;
use warnings;
use Net::Ping;
use CGI qw/:standard/;

my @hosts= -e 'ship.cfg' ? do {
    open my $cfgfile, '<', 'ship.cfg' 
      or die $!;
    map {chomp; $_ } <$cfgfile>;
} : qw/host1 host2 host3/;  # defaults

my (@up, @down);
my $p=Net::Ping->new('udp');
push @{ $p->ping($_) ? \@up : \@down }, $_ for @hosts;
$p->close();

print header, start_html, 
  start_form, submit 'Refresh';
    
my @verbage=qw/are hosts are hosts/;
@verbage[0,1]=qw/is host/ if @up == 1;
@verbage[2,3]=qw/is host/ if @down == 1;
    
print p, "There $verbage[2] ",
  (scalar @down),
  " $verbage[3] down" unless @down;

print p, "There $verbage[0] ",
  (scalar @up),
  " $verbage[1] alive" unless @up;

if (@up) {
    print p, "The following $verbage[1] $verbage[0] alive:\n", br;
    print li($_) for sort @up;
}

if (@down) {
    print p, "The following $verbage[3] $verbage[2] down:\n", br;
    print li($_) for sort @down;
}

__END__

[download]

Replies are listed 'Best First'.
Re^2: using -T doesn't work
by tcf03 (Deacon) on Dec 29, 2004 at 17:48 UTC
    Thanks for the help, Im slowly revising the code - I want to understand the code Im replacing before I replace it, so - so far this is where im at with the code. 
    use strict;
use warnings;
use Net::Ping;
use CGI qw/:standard/;
use POSIX qw(strftime);

# Clean up our UNIX environment
# for more infor read perldoc perlsec
$ENV{'PATH'} = '/bin:/usr/bin';
delete @ENV{qw(IFS CDPATH ENV BASH_ENV)};

my (@hosts, @uphosts, @downhosts);

if ( -e "ship.cfg" ) {
        open (CFGFILE, "ship.cfg") || die $!;
        while (my @TMP=<CFGFILE>) {
                #if ( $_ =~ /^#/ ) { next }
                # Lets untaint the data after making sure it only matc
+hes
                # word characters... For more info read perldoc perlse
+c.
                foreach $_(@TMP) {
                        if ($_ =~ /^([\w.]+)$/) {
                                $_ = $1;
                                push (@hosts, $1);
                        } else {
                                next;
                        }
                }
        }
        close (CFGFILE);
} else {
        # or enter your hosts here...
        # This is the most secure/preferred way to do this
        @hosts = qw/sapdev0 sapprd0 saptst0 pcbackup vader/;
}

# I haven't decided what else to do here - it just semantics...
my @verbage=("are", "hosts", "are", "hosts");

# udp is default
# other values can be icmp (if youre root)
# or TCP, which is expensive on the wire
my $p = Net::Ping->new('udp');

# Iterate through my hosts and get their status
push @{ $p->ping($_) ? \@uphosts : \@downhosts }, $_ for @hosts;
$p->close();

print header, start_html;

print "<tt>\n";
print "\n<center><h3>MSHIP - my status of host IP's</h3></center>\n";
my $now=strftime "%m/%d/%Y %H:%M:%S", (localtime);
print "<center>$now</center>\n";
print hr;

# Just cleaning up the verbage is are host hosts etc...
if (scalar(@downhosts) == 1 ) {
        $verbage[2]="is";
        $verbage[3]="host"
if (scalar(@uphosts) == 1 ) {
        $verbage[0]="is";
        $verbage[1]="host";
}

# We don't care about things that don't exist...
unless (scalar(@downhosts) == 0) { print p,"There $verbage[2] ",scalar
+(@downhosts),"  $verbage[3] unreachable\n",br }
print "There $verbage[0] ",scalar(@uphosts)," $verbage[1] alive\n";

print p,"The following $verbage[1] $verbage[0] alive: \n",br;
foreach my $uitem (sort @uphosts) { # sort is for cleanliness...
        print li("$uitem\n"),br;
}
# Again, we don't care about things that don't exist
unless (scalar(@downhosts) == 0) { print p,"The following $verbage[3] 
+$verbage[2] unreachable: \n",br }
foreach my $ditem (sort @downhosts) {
        print li("$ditem\n"),br;
}
print "</tt>\n";

print start_form,
        p,submit('Refresh'),
        end_html;

    [download]
    Where Im at now is on this piece of code: 
    # Iterate through my hosts and get their status
push @{ $p->ping($_) ? \@uphosts : \@downhosts }, $_ for @hosts;
$p->close();

    [download]

    what I get from this is if ping then either @uphosts (if successful) or @downhosts gets $_ from @host. Im just not getting why is it in the form of
    @{ $p->ping($_) ? \@uphosts : \@downhosts }
    and what does referencing @downhosts and @uphosts do that just calling them won't do?
    Or where can I read more on this other than perldata, perlref, etc...
    Thanks
    Ted
[reply]
[d/l]
[select]
      Thanks for the help, Im slowly revising the code - I want to understand the code Im replacing before I replace it, so - so far this is where im at with the code.
      Ok, Please note that to distinguish my code from yours, mine will be commented out (when in a paragraph by itself).

      Note: the code I proposed the last time was rewritten from yours according to my own personal preferences. I tried to keep its general structure as close as possible to yours, though. 

      my (@hosts, @uphosts, @downhosts);

if ( -e "ship.cfg" ) {
        open (CFGFILE, "ship.cfg") || die $!;

      [download]
      If you use a lexical filehandle then you do not need to close() it (when going out of scope (and if there are no more references to it)).

      Personally I like to avoid all parentheses altogether and use the low priority C<and> for flow control. I like to use the three-args from of open() even if it is not strictly necessary for I think it's a good habit. 

              while (my @TMP=<CFGFILE>) {

      [download]
      Perl's idiom for reading "lines" (whatever these can be according to $/) is 
              # while (<CFGFILE>) {

      [download]
      If you want to do *exactly* the same with an explicit variable rather than $_, then you have to do 
              # while (defined(my $line = <ARGV>)) {
        #     ...
        # }

      [download]
      What you wrote won't do what you think, since notwithstanding the C<while>, all of the file's content will be read into @TMP at a time. 
                      #if ( $_ =~ /^#/ ) { next }  # commented out in the OP

      [download]
      And $_ is?

      Also, while legal, you're missing the whole point about $_: it's the implicit variable on which so many operators and functions act. So you either want to $variable =~ /something/ or /something/ (if your variable is $_).

      Also, you may not like statement modifiers, but if there's a case in which one would come really handy is exactly this one: next if /^#/; (IMHO much more readable/immediate). 

                      foreach $_(@TMP) {

      [download]
      Don't! Quite similarly you either do something for my $var (@list) { ... or for (@list) { ... (if you want to use $_). 
                              if ($_ =~ /^([\w.]+)$/) {

      [download]
      Don't! 
                                      $_ = $1;

      [download]
      Why?!? 
                                      push (@hosts, $1);
                        } else {
                                next;
                        }

      [download]
      No need for the C<next> block, as if the condition is not fulfilled it will pass to the next iteration anyway. 
              close (CFGFILE);

      [download]
      If you use a lexical filehandles then you do not need to close() them. 
      # Just cleaning up the verbage is are host hosts etc...
if (scalar(@downhosts) == 1 ) {
        $verbage[2]="is";
        $verbage[3]="host"
if (scalar(@uphosts) == 1 ) {
        $verbage[0]="is";
        $verbage[1]="host";
}

      [download]
      As I wrote the other time, no need for scalar() there, and you can use an array slice. Not to say that this is not correct, strictly speaking, only IMHO it is much cleaner the other way round. For an actual example see the sample code I posted in the other node. 
      # We don't care about things that don't exist...
unless (scalar(@downhosts) == 0) { print p,"There $verbage[2] ",scalar

      [download]
      No need for the first scalar() as well as no need for the C<==0>. (No: this is not a mouse!! ;-)

      The same cmts apply to much of the following code, I'm not repeating them.

      Also, this is yet another one of those things that depend largely on personal preferences, but well written code should be self explanatory and only really relevant comments should be there. Commenting the obvious doesn't add to readability, IMHO: it has just the opposite effect instead!

      Where Im at now is on this piece of code: # Iterate through my hosts and get their status push @{ $p->ping($_) ? \@uphosts : \@downhosts }, $_ for @hosts; $p->close();

      what I get from this is if ping then either @uphosts (if successful) or @downhosts gets $_ from @host. Im just not getting why is it in the form of @{ $p->ping($_) ? \@uphosts : \@downhosts }

      Well, it's just a way to do it that I find to be convenient. Of course it's up to your personal preferences. You may consider it to be slightly ineffcient, but we're dealing with only a bunch of them anyway and $p->ping($_) takes most of the time here.
      and what does referencing @downhosts and @uphosts do that just calling them won't do? Or where can I read more on this other than perldata, perlref, etc...
      Hint: push() wants an array as its first argument, I'm giving it one. I think perlref has all that is needed to be known. Check perlreftut, as well.
[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom