David Douthitt writes:
| > Here's my code:
| > 
| > #!/opt/ruby/bin/ruby
| > 
| > class Lock

[...]

| >    def setlock (lockf = @lockfile)
| >       raise "Lockfile not set!" if lockf == nil;
| >       raise "Lock failed!" if
| >          ! system("set -o noclobber ; cat /dev/null 2> /dev/null > #{lockf}")
| 
| Why do you use such complicate system call here? I would prefer a Ruby 
| solution:
|         if not File.exist?(lockf)
|            begin File.open(lockf, "w") { |fp| fp.write(Process.pid) }
|            rescue
|               raise "Lock failed (system error)"
|            end
|         else
|            raise "Lock failed (already locked)"
|         end

This is not guaranteed to work.  This shows the danger of locks: in the
time between the test (File.exist?) and the creation (File.open) someone
else could interfere, and a lock could already be there when File.open
is reached.

The system call goes like this:

set -o noclobber          # Don't clobber files when redirected
cat /dev/null 2> /dev/null > lockfile   # create lock file, or fail if exists

No chance (I think) of someone getting the lock after we "test" it - since
the test is presumably at a system level.

| >       at_exit { self.unlock }
| >       @locked_up = true
| >    end

[...]

| >    def unlock
| >       test(?e, @lockfile) if
| >          raise "Unlock failed!" if
| >             ! system("rm -f #{@lockfile}");
| 
| What you are try to doing here? If 'rm -f' failed the exception is
| raised and 'test(?e,...)' will not performed. If 'rm' went well,
| neither 'raise' nor 'test' will be executed.
| 
|         raise "Unlock failed (no lock)" unless File.exist?(@lockfile)
|         begin
|           File.delete @lockfile
|         rescue
|           raise "Unlock failed (couldn't remove lockfile)"
|         end

I see I mangled my code :-)  What I want was probably more like this
(using your improvements nonetheless):

      return if ! File.exist?(@lockfile)
      begin
         File.delete @lockfile
      rescue
         raise "Unlock failed! (couldn't remove lockfile)"
      end

| Your code is ok! Only there are some things I would like to
| remark. 
| 
|    1. You often use constants like "/var/spool/locks" throughout your
|       whole code.

Definitely!  "/var/spool/locks" is meaningless.  @lockdir has much
more meaning.  It also allows you to change the directory easily.

   lockdir = "/var/spool/locks"

could be changed to

   lockdir = "/var/run"

or to

   lockdir = $*[0]

and so on....

|    2. You use too much system dependend things like system(...)
|       here. This it not really necessary.

When you gotta go, you gotta go :-)

|    3. You should not use things like 'test(?e, ...)'. The Ruby classes 
|       are more descriptive.

I'm still learning....

|    4. IMHO, your code is not carefully designed for the class/instance 
|       clash. Means, you do some things both in class methods and in
|       instance methods. 

I'm still finding out what I can do and can't.  Tell me more about
this class/instance distinction.  I'm forever finding myself wanting
"a bunch of X" where X inevitably turns out to be an instance
and then the first code becomes

   Xes = Hash.new

Is this typical?

|    5. You should not on any case define accessors for your class. I
|       would consider it not-so-good style if I would have to:
| 
|       lock = Lock.new("/var/spool/mail/cle")
|       lock.lockdir = "/var/spool/mail"
|       lock.setlock
| 
|       following would be better, IMO:
| 
|       lock = Lock.new("/var/spool/mail/cle", "/var/spool/mail")
|       lock.setlock

Ahhhhh..... I was wondering some about that.  I have accessors
almost everywhere :-(

| All in all I would say, that your code may be appropiate for what you
| have in mind for your apps. But it is not generic enough, IMHO! No
| steal-the-lock feature. No wait for the lock. No auto-unlock as soon
| as I do not need this file instance anymore...

Steal-the-lock?  HORRORS!

Wait for the lock?  Interesting....

Auto-unlock?  Already there.  Check for the "at_exit" call, as well as the
locked method.  locked is nice:

mylock.locked {
   ...do something while locked...
   }

....now unlocked....

One think I like about my Lock is that the names are similar to those
of the Thread module - so one less thing to have to learn.