http://qs321.pair.com?node_id=125498


in reply to Looking for feed back on a guestbook

Your script has a race condition. That is, two copies of your script can be running at the same time and they both can try to write a new entry to the guestbook, resulting in it being curropted. To fix this problem you can use flock. (Just use Fcntl qw(:flock); ... flock HTML, LOCK_EX; would probably be enough, see the docs for details.)

A more minor issue with your script is that print start_html() will print out <html> and a <head> section for your document. You probably shouldn't use it, unless you plan to use CGI to generate all your HTML. (Especially considering that CGI will output XHTML in newer versions, making it so browsers would be welcome to reject your apparently well-formed HTML.)

I notice that you're testing variables for a pattern and then preforming subtitutions for that pattern. It would be clearer, and probably just as fast, to just try the subtitution -- if the pattern's not found, nothing will happen. Personally, I would write your repetitive substitutions of <, etc. for the appropriate entities with a foreach loop:

foreach ($name,$mail,$message) { s/</&lt;/g; s/>/&gt;/g; }

A final potential problem in your script is that you might want to think of a better way of handling situtations where certain input fields are blank.

In your use of CGI.pm you are mixing the object-oriented ($q->something) and function-based (something()) interfaces. It would be clearer if you stuck with only one.

update: I notice you use #!/use/bin/perl -w. I suspect you meant #!/usr/bin/perl -w