[mythtv-commits] Ticket #3248: mytharchive lockfile issues

[MythTV]

#3248: mytharchive lockfile issues
-------------------------+--------------------------------------------------
 Reporter:  anonymous    |        Owner:  paulh  
     Type:  defect       |       Status:  new    
 Priority:  minor        |    Milestone:  unknown
Component:  mytharchive  |      Version:  0.20   
 Severity:  medium       |   Resolution:         
-------------------------+--------------------------------------------------

Comment(by anonymous):

 Thanks, PaulH, for changeset 13070.

 There is still a bug in mythburn.py: the mythburn.lck file is removed in
 line 4039 even if the lockfile is not owned by this process (i.e. was not
 created by this process).  The obvious fix is to move lines 3997 through
 4000 outside the try that starts at line 3996.

 In actual fact, those lines ought to be redundant.  Line 4002, if done
 right, would eliminate the need for them.  Unfortunately, it isn't quite
 right and leaves a small race condition.

 The function call os.open(os.path.join(logpath, "mythburn.lck"),
 os.O_WRONLY | os.O_CREAT | os.O_EXCL) will fail if the lock file exists
 and create it if it does not exist (unless there is another problem such
 as the directory not existing, or not being writeable, or the file system
 being full, etc.).  The correct form of this line should be something like
 the following (replacing lines 3998 through 4004:


 {{{
 import errno
 lckpath = os.path.join(logpath, "mythburn.lck")
 try:
     fd = os.open(lckpath, os.O_WRONLY | os.O_CREAT | os.O_EXCL)
     try:
         os.write(fd, "%d\n" % os.getpid())
         os.close(fd)
     except:
         os.remove(lckpath)
         raise
 except OSError, e:
     if e.errno == errno.EEXIST:
         write("Lock file exists -- already running???")
         sys.exit(1)
     else:
         fatalError("cannot create lockfile: %s" % e)
 # if we get here, we own the lock

 }}}

 The inner try is there in the hopes of making the code more bulletproof.
 The failure would probably only happen if the disk drive were full.

 At this point, the "try" from line 3996 should be moved to after this
 block of code.  That is the point at which we know the lockfile is ours
 and hence we should remove it when we are done.

 I've used "fd" instead of "file" to hold the file descriptor.  I think
 that this makes the code clearer.

 I've used "lckpath" to hold the lock file path.  Since this is used
 several places, I think it would be clearer and less subject to bitrot to
 form it in one place in the code.  Other places should reference this
 variable too (lines 4038 and 4039).  Actually, line 4038 ought to now be
 redundant: 4039 should be unconditional since we know that we've got the
 lock file.

 I've added a \n to the format for the contents of the lockfile.  If I
 correctly understand the code in mytharchive/mail.cpp:checkProcess, it
 seems to think that the lockfile contains a line.

 Speaking of checkProcess, the test for the existence of a process is
 perhaps not the best.  The normal way, in POSIX, is to test with
 kill(pid,0) system call and see if the result is ESRCH (meaning "no
 process can be found corresponding to that pid").

-- 
Ticket URL: <http://svn.mythtv.org/trac/ticket/3248#comment:1>
MythTV <http://svn.mythtv.org/trac>
MythTV