Re: [Mailman-Developers] patch candidate for MailList.Save()
[Barry A. Warsaw]
The idea is that the marshal is first written to a temp file called config.db.tmp.<mypid>. If this succeeds completely, then
1) config.db.last is unlinked 2) a hard link config.db.last -> config.db is created 3) a hard link config.db -> config.db.tmp.<mypid> is created 4) config.db.tmp.<mypid> is unlinked
Comments?
You missed 2.5) config.db is removed which constitutes a race condition -- I think the below patch fixes that (but as you say, this really is crucial code to get right, so I'm posting it here for others to comment before committing): Index: MailList.py =================================================================== RCS file: /export/public/cvsroot/mailman/Mailman/MailList.py,v retrieving revision 1.130 diff -u -r1.130 MailList.py --- MailList.py 1999/07/18 17:26:55 1.130 +++ MailList.py 1999/07/19 15:09:08 @@ -759,6 +759,8 @@ fname = os.path.join(self._full_path, 'config.db') fname_tmp = fname + '.tmp.' + `os.getpid()` fname_last = fname + ".last" + # Make config.db unreadable by `other', as it contains all the + # list members' passwords (in clear text). omask = os.umask(007) try: try: @@ -771,10 +773,14 @@ 'Failed config.db file write, retaining old state' '\n %s' % `status.args`) Utils.reraise() - # now move config.db -> config.db.last - # then move config.db.tmp.xxx -> config.db - aside_new(fname, fname_last) - aside_new(fname_tmp, fname) + # Now do config.db.tmp.xxx -> config.db -> config.db.last + # rotation -- safely. + if os.path.exists(fname_last): + os.unlink(fname_last) + os.link(fname, fname_last) + os.rename(fname_tmp, fname) +## aside_new(fname, fname_last) +## aside_new(fname_tmp, fname) finally: os.umask(omask) self.CheckHTMLArchiveDir() After this change, the function aside_new() in MailList.py can be removed (or moved to Utils.py), as it isn't being used anywhere else. -- Harald
"HM" == Harald Meland <Harald.Meland@usit.uio.no> writes:
HM> You missed
HM> 2.5) config.db is removed
HM> which constitutes a race condition -- I think the below patch
HM> fixes that (but as you say, this really is crucial code to get
HM> right, so I'm posting it here for others to comment before
HM> committing):Arg! You're right of course. Here's a slightly reworked version of your patch. Also, not committed yet, but seems to work for me.
-Barry
-------------------- snip snip -------------------- Index: MailList.py
RCS file: /projects/cvsroot/mailman/Mailman/MailList.py,v retrieving revision 1.130 diff -c -r1.130 MailList.py *** MailList.py 1999/07/18 17:26:55 1.130 --- MailList.py 1999/07/19 22:22:56
*** 26,31 **** --- 26,32 ---- 'for the site?')
import sys, os, marshal, string, posixfile, time
- import errno import re import Utils import Errors
*** 759,764 ****
--- 760,767 ----
fname = os.path.join(self._full_path, 'config.db')
fname_tmp = fname + '.tmp.' + os.getpid()
fname_last = fname + ".last"
# Make config.db unreadable by `other', as it contains all the# list members' passwords (in clear text). omask = os.umask(007) try: try:
*** 771,780 ****
'Failed config.db file write, retaining old state'
'\n %s' % status.args)
Utils.reraise()
! # now move config.db -> config.db.last
! # then move config.db.tmp.xxx -> config.db
! aside_new(fname, fname_last)
! aside_new(fname_tmp, fname)
finally:
os.umask(omask)
self.CheckHTMLArchiveDir()
--- 774,789 ----
'Failed config.db file write, retaining old state'
'\n %s' % status.args)
Utils.reraise()
! # Now do config.db.tmp.xxx -> config.db -> config.db.last
! # rotation -- safely.
! try:
! # might not exist yet
! os.unlink(fname_last)
! except os.error, (code, msg):
! if code <> errno.ENOENT:
! Utils.reraise()
! os.link(fname, fname_last)
! os.rename(fname_tmp, fname)
finally:
os.umask(omask)
self.CheckHTMLArchiveDir()
*** 1376,1397 ****
return ("<%s.%s %s%s at %s>"
% (self.__module__, self.__class__.__name__,
self._internal_name, status, hex(id(self))[2:]))
- def aside_new(old_name, new_name, reopen=0):
"""Utility to move aside a file, optionally returning a fresh open version.We ensure maintanance of proper umask in the process."""# Make config.db unreadable by `other', as it contains all the list# members' passwords (in clear text).ou = os.umask(007)try:if os.path.exists(new_name):os.unlink(new_name)if os.path.exists(old_name):os.link(old_name, new_name)os.unlink(old_name)if reopen:file = open(old_name, 'w')return filefinally:
--- 1385,1387 ----os.umask(ou)
participants (2)
-
Barry A. Warsaw -
Harald Meland