[boulder.pm] advisory locking

Joel Maslak jmaslak at mindspring.com
Mon Jan 8 17:23:15 CST 2001


Appologizes if this is a dupe.  I sent it from the wrong account and the
anti-spam stuff will probably kill the other one.

Walter asked for volunteers to pick through the code, so I volunteered...  
I'd love to know what I missed!  Thanks for giving us the opportunity,
Walter, to pick on your code so that we might all learn a little bit.

I'll give my thoughts on the code.  I'm not going to cut any of it except
the preamble stuff so that there is sufficiant context.

On Thu, 4 Jan 2001, Walter Pienciak wrote:

> my $LOCKDIR = '/var/tmp';
> my $EDITOR  = '/bin/vi';                        #  Default editor.
> $EDITOR = $ENV{EDITOR} if ($ENV{EDITOR} ne ''); #  But not everyone likes it.

Should also check $ENV{VISUAL}...

> my $file;
> while (scalar @ARGV > 0) {
>     $file = shift;

The more "perlish" way (if there is one with TMTOWTDI):
foreach $file (@ARGV) {
	file_lock($file);
	...

 
>     &file_lock($file);

Note that the "&" is unnecessary on function calls when you enclose the
parameters in parentheses.  This is a style issue, though, and not a bug
or problem in the code.

>     system("$EDITOR $file");

What if the filename of the editor or the file being edited has a space in
it?  This will fail if it does.  I would replace this with:
	system($EDITOR, $file);

I would also check the return value of the editor and use it to deterime
the return value of your program (if all edits returned 0, return 0;
otherwise, return some error value).  That way, if it is called by yet
another program, and the editor dies for some reason, then the calling
program will know that the edit might need to be handled specially.

>     &file_unlock($file);
> }
>
> sub file_lock {
>     my $f = shift;
>     sysopen(LOCKFILE, "$LOCKDIR/LOCK_$f", O_WRONLY | O_CREAT)
>         or die "Can't open $LOCKDIR/LOCK_$f: $!";

This is a problem.  Let's say that I try to "newvi /etc/hosts".  Unless
LOCK_ is a directory and LOCK_/etc is also one, this will fail.

>     flock(LOCKFILE, LOCK_EX|LOCK_NB)            #  Nonblocking lock.
>         or die "Can't obtain lock for $LOCKDIR/LOCK_$f: $!";
> }

This lets someone "lock" a file that they can't even really edit.  That
might not be what you want.

Of course I must ask, why not just lock the file you are editing
directly?  I'm assuming that there is a reason for creating the lock file
 -- I just don't see it.

> sub file_unlock {
>     my $f = shift;
>     unlink "$LOCKDIR/LOCK_$f"
>         or die "Can't delete $LOCKDIR/LOCK_$f: $!";
>     flock(LOCKFILE, LOCK_UN)
>         or die "Can't release lock for $LOCKDIR/LOCK_$f: $!";
> }

You should probably close LOCKFILE, too.

> ##  Th-th-that's all, folks!

-- 
Joel










More information about the Boulder-pm mailing list