Let's make this a little nicer:
#!/usr/bin/perl -w
use strict; # you're safer with this
use CGI;
my $query = new CGI;
print "Content-type: text/html\n\n";
my $name = $query->param('name');
my $file;
{ # let's be safe here with filehandles, too
local *FILE;
open (FILE, $name) || error("Can't find $name: $!");
my $first_line = <FILE>;
chomp ($file = <FILE>); # takshaka gets bonus points for ac
+tually testing it
close FILE;
}
if ($file eq $query->param('pass')){ # more logically phrased
print qq[<HTML>
<frameset cols="100%,290%">
<frame src="commands.cgi?name=$name];
print qq[" name="commands">
<frame src="base.cgi?name=$name];
print qq[" name="game">
</frameset>
</HTML>];
} else {
error("Sorry, that was the wrong pass/name, please try again.");
}
sub error {
my $text = shift;
print qq[<HTML>$text</HTML>];
exit;
}
There, that ought to work better. You did a couple of (syntactially valid, yes) things that could come back to haunt you, so I cleaned them up.
First, you read the file in list context, into an array. If you just want the second line of it, we'll throw the first one away into a temporary variable and grab the second in another statement. That'll save reading the whole thing in if it's a potentially large file. (I don't particularly like the temporary variable throwaway thing here, but it's a good way to illustrate the point. @foo1 will give warnings with strict because it ought to be written $foo1 as it returns a scalar, not a list.)
Notice also the chomp statement, to remove the input record separator (\n, a newline) from $file. That'll make things match successfully. Most of the other stuff just makes it work under the strict pragma. There's also a little more error checking. Be sure to do that.
I like to wrap my open calls in blocks so I can use local on the filehandles. It doesn't matter much in small scripts, but as they have a way of growing into larger files, if you already have a filehandle named FILE active somewhere else, you'll clobber it here.
And, yes KM, there ought to be an exit at the end of the error sub, so let's redact that in. :) | [reply]
[d/l] |
| [reply]
[d/l] |
Why the use of the block to open the file? Why the use of local *FILE here? Why are you making two variables instead of just @foo = <FILE>; $foo[1];? Why not:
print qq[<HTML><frameset cols="100%,290%">
<frame src="commands.cgi?name=$name" name="commands">
<frame src="base.cgi?name=$name" name="game">
</frameset>
</HTML>];
?
Why not exit at the end of error()? Otherwise the script will continue, which would make no sense, and possibly cause other errors.
Cheers,
KM | [reply]
[d/l] |
Why are you making two variables instead
of just @foo = <FILE>; $foo[1];?
I know the data file probably has only a couple of
lines at most, but if it had 1000 why would you
want to read in all 1000 when you only want the second?
Of course, using two scalars is wasteful.
my $line = (<FILE>, <FILE>);
I suspect the OP really wants the first line--not
the second--which would make slurping into an array
even more unnecessary.
| [reply]
[d/l] |
First, you should always check to make sure the open worked:
open(FILE, $query->param('name')) or die "Can't open file";
Actually, I would not open any file directly from user input. Others may disagree in this case, but I would validate that param('name') is something you expect, likely /^\w+$/, or something similar.
If you had warnings turned on (which you should, -w) you would see that @file[1] is better written as $file[1]. You may want to consider putting all the HTML you are printing in a here doc, what you have is pretty hard on the eyes. As for why they do not match, you may want to chomp() the line you get from the file. If the file only contains one line, you should use this:
open(FILE, $name) or die "Can't open $name";
$pass = <FILE>;
close FILE;
chomp $pass;
if ($query->param('pass') eq $pass) {
...etc...
Cheers,
KM | [reply]
[d/l]
[select] |