From b6ba7955acf6b1046991636a82bbb8387f3e4e28 Mon Sep 17 00:00:00 2001 From: Jeremy Fincher Date: Sun, 1 Aug 2004 12:46:03 +0000 Subject: [PATCH] With some clearer thinking, I believe this is the proper implementation of tmpDir. If there's something wrong with it, send me a note or write a test and it'll be fixed. --- plugins/Infobot.py | 2 +- plugins/URL.py | 2 +- src/cdb.py | 2 +- src/conf.py | 17 ++++++++++++----- src/ircdb.py | 6 +++--- src/registry.py | 2 +- src/utils.py | 45 +++++++++++++++++++++++++++------------------ 7 files changed, 46 insertions(+), 30 deletions(-) diff --git a/plugins/Infobot.py b/plugins/Infobot.py index 1daab6986..a7d87a73f 100755 --- a/plugins/Infobot.py +++ b/plugins/Infobot.py @@ -104,7 +104,7 @@ class InfobotDB(object): 'I hear ya'] def flush(self): - fd = utils.AtomicFile(filename) + fd = utils.transactionalFile(filename, 'wb') pickle.dump((self._is, self._are), fd) fd.close() diff --git a/plugins/URL.py b/plugins/URL.py index 0247ee784..85e4a7058 100644 --- a/plugins/URL.py +++ b/plugins/URL.py @@ -149,7 +149,7 @@ class URLDB(object): return [url for (url, nick) in self.getUrlsAndNicks(p)] def vacuum(self): - out = utils.AtomicFile(self.filename) + out = utils.transactionalFile(self.filename) notAdded = 0 urls = self.getUrlsAndNicks(lambda *args: True) seen = sets.Set() diff --git a/src/cdb.py b/src/cdb.py index 78b07a9b4..65d1db1fb 100644 --- a/src/cdb.py +++ b/src/cdb.py @@ -136,7 +136,7 @@ def make(dbFilename, readFilename=None): class Maker(object): """Class for making CDB databases.""" def __init__(self, filename): - self.fd = utils.AtomicFile(filename) + self.fd = utils.transactionalFile(filename) self.filename = filename self.fd.seek(2048) self.hashPointers = [(0, 0)] * 256 diff --git a/src/conf.py b/src/conf.py index ec9d45b0a..a6f52852c 100644 --- a/src/conf.py +++ b/src/conf.py @@ -526,11 +526,19 @@ registerGlobalValue(supybot.drivers, 'module', class Directory(registry.String): def __call__(self): + # ??? Should we perhaps always return an absolute path here? v = super(Directory, self).__call__() if not os.path.exists(v): os.mkdir(v) return v + def dirize(self, filename): + dir = self() + dirname = os.path.basename(filename) + if not dirname.endswith(dir): # ??? Should this be an "in" test instead? + return os.path.join(dir, os.path.basename(filename)) + return filename + class DataFilename(registry.String): def __call__(self): v = super(DataFilename, self).__call__() @@ -557,12 +565,11 @@ registerGlobalValue(supybot.directories.data, 'tmp', DataFilenameDirectory('tmp', """Determines what directory temporary files are put into.""")) -# This is a hack, but it should work. -utils._AtomicFile = utils.AtomicFile -def AtomicFile(*args, **kwargs): +# Remember, we're *meant* to replace this nice little wrapper. +def transactionalFile(*args, **kwargs): kwargs['tmpDir'] = supybot.directories.data.tmp() - return utils._AtomicFile(*args, **kwargs) -utils.AtomicFile = AtomicFile + return utils.AtomicFile(*args, **kwargs) +utils.transactionalFile = transactionalFile class PluginDirectories(registry.CommaSeparatedListOfStrings): def __call__(self): diff --git a/src/ircdb.py b/src/ircdb.py index 4d0f990c6..f6b2217dd 100644 --- a/src/ircdb.py +++ b/src/ircdb.py @@ -571,7 +571,7 @@ class UsersDictionary(utils.IterableMap): if self.filename is not None: L = self.users.items() L.sort() - fd = utils.AtomicFile(self.filename) + fd = utils.transactionalFile(self.filename) for (id, u) in L: fd.write('user %s' % id) fd.write(os.linesep) @@ -732,7 +732,7 @@ class ChannelsDictionary(utils.IterableMap): def flush(self): """Flushes the channel database to its file.""" if self.filename is not None: - fd = utils.AtomicFile(self.filename) + fd = utils.transactionalFile(self.filename) for (channel, c) in self.channels.iteritems(): fd.write('channel %s' % channel) fd.write(os.linesep) @@ -792,7 +792,7 @@ class IgnoresDB(object): def flush(self): if self.filename is not None: - fd = utils.AtomicFile(self.filename) + fd = utils.transactionalFile(self.filename) for hostmask in self.hostmasks: fd.write(hostmask) fd.write(os.linesep) diff --git a/src/registry.py b/src/registry.py index 4f4b32a90..f86d16b9a 100644 --- a/src/registry.py +++ b/src/registry.py @@ -78,7 +78,7 @@ def open(filename, clear=False): def close(registry, filename, annotated=True, helpOnceOnly=False): first = True helpCache = sets.Set() - fd = utils.AtomicFile(filename) + fd = utils.transactionalFile(filename) for (name, value) in registry.getValues(getChildren=True): if annotated and hasattr(value,'help') and value.help: if not helpOnceOnly or value.help not in self.helpCache: diff --git a/src/utils.py b/src/utils.py index 182264063..6432ee6f1 100755 --- a/src/utils.py +++ b/src/utils.py @@ -673,34 +673,36 @@ def stackTrace(): class AtomicFile(file): """Used for files that need to be atomically written -- i.e., if there's a - failure, the original file remains, unmodified. - - Opens the file in 'w' mode.""" - def __init__(self, filename, allowEmptyOverwrite=False, tmpDir=None): - self.filename = os.path.abspath(filename) + failure, the original file remains, unmodified. mode must be 'w' or 'wb'""" + def __init__(self, filename, mode='w', + allowEmptyOverwrite=False, tmpDir=None): + if mode not in ('w', 'wb'): + raise ValueError, 'Invalid mode: %r' % mode self.rolledback = False self.allowEmptyOverwrite = allowEmptyOverwrite - self.tempFilename = '%s.%s' % (self.filename, mktemp()) - if tmpDir is not None: - tempFilename = os.path.dirname(self.tempFilename) - self.tempFilename = os.path.join(tmpDir, self.tempFilename) - super(self.__class__, self).__init__(self.tempFilename, 'w') + self.filename = filename + if tmpDir is None: + # If not given a tmpDir, we'll just put a random token on the end + # of our filename and put it in the same directory. + self.tempFilename = '%s.%s' % (self.filename, mktemp()) + else: + # If given a tmpDir, we'll get the basename (just the filename, no + # directory), put our random token on the end, and put it in tmpDir + tempFilename = '%s.%s' % (os.path.basename(self.filename), mktemp()) + self.tempFilename = os.path.join(tmpDir, tempFilename) + super(AtomicFile, self).__init__(self.tempFilename, mode) def rollback(self): - #print 'AtomicFile.rollback' if not self.closed: - super(self.__class__, self).close() + super(AtomicFile, self).close() if os.path.exists(self.tempFilename): - #print 'AtomicFile: Removing %s.' % self.tempFilename os.remove(self.tempFilename) self.rolledback = True def close(self): - #print 'AtomicFile.close' if not self.rolledback: - #print 'AtomicFile.close: actually closing.' - super(self.__class__, self).close() - size = os.stat(self.tempFilename).st_size + super(AtomicFile, self).close() + size = os.path.getsize(self.tempFilename) if size or self.allowEmptyOverwrite: if os.path.exists(self.tempFilename): # We use shutil.move here instead of os.rename because @@ -710,9 +712,16 @@ class AtomicFile(file): shutil.move(self.tempFilename, self.filename) def __del__(self): - #print 'AtomicFile.__del__' + # We rollback because if we're deleted without being explicitly closed, + # that's bad. We really should log this here, but as of yet we've got + # no logging facility in utils. I've got some ideas for this, though. self.rollback() +def transactionalFile(*args, **kwargs): + # This exists so it can be replaced by a function that provides the tmpDir. + # We do that replacement in conf.py. + return AtomicFile(*args, **kwargs) + if __name__ == '__main__': import sys, doctest doctest.testmod(sys.modules['__main__'])