Beefy Boxes and Bandwidth Generously Provided by pair Networks
Clear questions and runnable code
get the best and fastest answer

comment on

( #3333=superdoc: print w/replies, xml ) Need Help??

A few points. When locking a file I use this little snippet to allow a script to wait for its lock:

my $flock = 1; # set to true to flock my $timeout = 10; # set to number of seconds before flock timeout .... if ($flock) { my $count = 0; until (flock FILE, LOCK_SH) { sleep 1; DieNice("Can't lock file '$file': $!\n") if ++$count >= $timeout +; } }

Also closing a filehandle removes the underlying file lock so you only need to close them, Perl will unlock them for you thus all the LOCK_UN lines are redundant.

This line does nothing useful, it does not untaint $mode

$mode =~ tr#a-z_##cd; # untaint that param!

This bit uses $mode in a regex and the value in $1 assigned to $page is untainted.

if ( $mode =~ /^([a-z_]+)_go$/ ) { $page = $1 or die "no page given!";

The or die makes no logical sense as the assignment $page = $1 will always succeed thus this can never execute.

You have a number of or die "blah" statements. You should include the $! special var as this stores Perl's explanation of the error that has triggered the die ie "File does not exist". The standard synatax goes like:

open FILE, ">>$file" or die "Can't append to $file, Perl says $!\n";

This code is a very obtuse way to do something quite simple, namely go to one of two subs depending on the user input.

sub main { my %dispatch = ( upload => \&upload, debug => \&debug, DEFAULT => \&upload, ); $draw->headers(); $draw->html_start( { title => $title } ); my $action = $cgi->param('action') || 'DEFAULT'; my $do_sub = $dispatch{ $action }; if ( $do_sub ) { &$do_sub(); } else { warn "[warn] Invalid action: [$action]"; die "[die] You can't make me do that!\n"; } $draw->html_end(); }

You create a hash of sub references and then eventually call them. This is much easier to follow:

sub main { $draw->headers(); $draw->html_start( { title => $title } ); if ($cgi->param('action') eq 'debug'){ &debug; } else { &upload; } $draw->html_end(); }

The logic is the same with the exception of not checking for 'incorrect' action params. This is the typical logic I use as it is simple - either you ask for something with the correct name or you get the default page. The use of warn and die as you do is also redundant. Both warn and die print to STDERR. Warn just does this whereas die throws an expception afterwards. Logically you are just doing a die here.

Your open files in two different places in the script then lock them all in the one place. This makes no logical sense to me and makes it hard to follow your logic train.

Finally in CGI die is oftem suboptimal. You should not run a production script with use CGI::Carp qw( fatalsToBrowser ) active as it makes it easier to hack a script as the exact errors are reported in the browser. Without carp die will give the user a 500 error when it gets called. For these reasons amongst others most developers write a die_nice routine. Here is a die_nice routine:

sub DieNice { my $message = shift; my ($package, $file, $line) = caller(); my $user_message = "Sorry, the system is currently unable to proce +ss your request<br>\n"; $user_message .= "due to routine maintenance. Please try again lat +er. Our Apologies\n"; $message = Unindent <<" MESSAGE"; A fatal die was trapped by the $scriptname die nice routine, detai +ls: Time: $datetime List name: $list_name Script name: $scriptname Package: $package File: $file Line Number: $line Error Message: $message MESSAGE &TellUser($user_message); # the bullshit message! &WarnAdmin($message); # the real facts! exit; } sub WarnAdmin { my $message = shift; my $name = "Administrator"; my $email = $our_email; &EmailUser ($name, $email, $message); return; } sub Unindent { my $unindent = shift; $unindent =~ s/^[ \t]+//gm; return $unindent; }

The unindent sub just lets me indent the herepage stuff with the sub and drop this indentation off the final output. Hope this helps, all up looks good with -Tw, use strict, setting a secure path etc.




In reply to Re: Code review, good 'ol CGI. by tachyon
in thread Code review, good 'ol CGI. by meonkeys

Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":

  • Are you posting in the right place? Check out Where do I post X? to know for sure.
  • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
    <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
  • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
  • Want more info? How to link or How to display code and escape characters are good places to start.
Log In?

What's my password?
Create A New User
Domain Nodelet?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others rifling through the Monastery: (1)
As of 2023-06-01 06:24 GMT
Find Nodes?
    Voting Booth?

    No recent polls found