Re^2: Bad code from the trenches

First off, I feel the need to be a little defensive and say "This ain't *my* code!" It was in a module written by the head developer of my current client.

Ok, with that out of the way....

Your suggestion about picking from a list of acceptable files is a good one, but wouldn't work in this case (because of information that was not given in the original post). This code was used in a CGI environment and $dest was derived from a CGI param, but this wasn't code that was directly user-interactive, it was called from a PHP script.

As to testing files with -e, -f, and -d before opening them...remember that doing this:

    line 1)  if ( -e $file && -f $file && -s $file < $ok ) {
    line 2)     ...open file here....
             }
[download]

means that you have a race condition. If someone modifies or deletes $file in the moment between lines 1 and 2, the tests were all for nothing. A better solution is to do something like this (almost directly lifted from perlopentut):

    use 5.004;
    use Fcntl qw(:DEFAULT :flock);
    open(FH, '<', $file)  or die "can't open $file: $!";
    unless (flock(FH, LOCK_SH | LOCK_NB)) {
    local $| = 1;
    print "Waiting for lock...";
    flock(FH, LOCK_SH)  or die "can't lock $file: $!";
    print "got it.\n"
    } 
    # now read from FH
[download]

Comment on Re^2: Bad code from the trenches Select or Download Code

Replies are listed 'Best First'.
Re^3: Bad code from the trenches by dragonchild (Archbishop) on Mar 14, 2005 at 14:36 UTC
The problem is that not all OSes allow you to treat a directory as a normal file. Unix-like systems (including OS-X, but not OS-9) do, but Windows probably won't and VMS definitely won't (AFAIK). What happens then? As for the code being yours or not ... don't feel defensive. If it's not yours, you don't have anything to worry about. If it is yours, you've taken the right step(s) to fix your error(s) and learn better programming techniques. Either way, you have no fault. As for the entire pick-a-file thing ... there should still be a list of permissible files being presented within the PHP. At that point, both the Perl and the PHP need to agree on what this list of files is so that the PHP can present the list and the Perl can validate against it. Just because you have more than one code file doesn't mean the solution is wrong. Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply]
Re^4: Bad code from the trenches by Whitehawke (Pilgrim) on Mar 14, 2005 at 20:23 UTC
As for the entire pick-a-file thing ... there should still be a list of permissible files being presented within the PHP. At that point, both the Perl and the PHP need to agree on what this list of files is so that the PHP can present the list and the Perl can validate against it. Just because you have more than one code file doesn't mean the solution is wrong. Sorry, I was unclear. It certainly doesn't make it wrong, but I don't have control over the PHP files, so it isn't a solution I can implement. For someone else, who is taking this discussion as advice on how to write a green-field project, it would certainly be a good way to go.	[reply]