using -T doesn't workAt 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...
Good!use strict; use warnings;
Also good.use Net::Ping; use CGI qw/:standard/;
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...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/; }
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') { # ... }
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!
No need for C<()>.my @uphosts=(); my @downhosts=();
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.my $proto; my @verbage=("are", "hosts", "are", "hosts");
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.
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.my $p = Net::Ping->new('udp'); foreach my $host(@hosts) { chomp($host); if ($p->ping($host)) { push (@uphosts, $host); } else { push (@downhosts, $host); } }
Huh?!? This is not a Perl issue, but do you really want to print C<header> after C<start_html>?!?$p->close(); print start_html,br; print header,br; print start_form,br; print submit('Refresh'),p;
No need for scalar().if (scalar(@downhosts) == 1 ) { $verbage[2]="is"; $verbage[3]="host" } if (scalar(@uphosts) == 1 ) { $verbage[0]="is"; $verbage[1]="host"; }
You can use an array slice.
No need for the first scalar.unless (scalar(@downhosts) == 0) { print p,"There $verbage[2] ",scalar
Ditto as above wrt the topicalizer: you can also use C<for> as a statement modifier.(@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; }
UPDATE: please note that there's non need to quote all variables, see 'perldoc -q quoting'.
Ditto as above wrt scalar().unless (scalar(@downhosts) == 0) { print p,"The following $verbage[3] $verbage[2] down: ",br } foreach my $ditem (sort @downhosts) { print li("$ditem"),br; }
Ditto as above wrt the topicalizer.
All in all I'd rewrite the code as follows:
#!/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__
In reply to Re: using -T doesn't work
by blazar
in thread using -T doesn't work
by tcf03
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |