# HG changeset patch # User Julien Cristau # Date 1403535275 -7200 # Node ID 76f601a90aa7079fe65c00b54cecadb268684cf4 # Parent f3dec653a27d2e7c98a265ffd5fbacf572772cfa [storage] use mkstemp to create files in bfss Avoids race condition, at the cost of slightly less predictable file names. diff -r f3dec653a27d -r 76f601a90aa7 server/sources/storages.py --- a/server/sources/storages.py Thu Jun 11 09:51:30 2015 +0200 +++ b/server/sources/storages.py Mon Jun 23 16:54:35 2014 +0200 @@ -21,6 +21,7 @@ import sys from os import unlink, path as osp from contextlib import contextmanager +import tempfile from yams.schema import role_name @@ -84,20 +85,11 @@ # * handle backup/restore def uniquify_path(dirpath, basename): - """return a unique file name for `basename` in `dirpath`, or None - if all attemps failed. - - XXX subject to race condition. + """return a file descriptor and unique file name for `basename` in `dirpath` """ - path = osp.join(dirpath, basename.replace(osp.sep, '-')) - if not osp.isfile(path): - return path + path = basename.replace(osp.sep, '-') base, ext = osp.splitext(path) - for i in xrange(1, 256): - path = '%s%s%s' % (base, i, ext) - if not osp.isfile(path): - return path - return None + return tempfile.mkstemp(prefix=base, suffix=ext, dir=dirpath) @contextmanager def fsimport(session): @@ -122,16 +114,13 @@ # 0444 as in "only allow read bit in permission" self._wmode = wmode - def _writecontent(self, path, binary): + def _writecontent(self, fd, binary): """write the content of a binary in readonly file - As the bfss never alter a create file it does not prevent it to work as - intended. This is a beter safe than sorry approach. + As the bfss never alters an existing file it does not prevent it from + working as intended. This is a better safe than sorry approach. """ - write_flag = os.O_WRONLY | os.O_CREAT | os.O_EXCL - if sys.platform == 'win32': - write_flag |= os.O_BINARY - fd = os.open(path, write_flag, self._wmode) + os.fchmod(fd, self._wmode) fileobj = os.fdopen(fd, 'wb') binary.to_file(fileobj) fileobj.close() @@ -155,10 +144,10 @@ entity._cw_dont_cache_attribute(attr, repo_side=True) else: binary = entity.cw_edited.pop(attr) - fpath = self.new_fs_path(entity, attr) + fd, fpath = self.new_fs_path(entity, attr) # bytes storage used to store file's path entity.cw_edited.edited_attribute(attr, Binary(fpath)) - self._writecontent(fpath, binary) + self._writecontent(fd, binary) AddFileOp.get_instance(entity._cw).add_data(fpath) return binary @@ -189,10 +178,9 @@ fpath = None else: # Get filename for it - fpath = self.new_fs_path(entity, attr) - assert not osp.exists(fpath) + fd, fpath = self.new_fs_path(entity, attr) # write attribute value on disk - self._writecontent(fpath, binary) + self._writecontent(fd, binary) # Mark the new file as added during the transaction. # The file will be removed on rollback AddFileOp.get_instance(entity._cw).add_data(fpath) @@ -224,13 +212,13 @@ name = entity.cw_attr_metadata(attr, 'name') if name is not None: basename.append(name.encode(self.fsencoding)) - fspath = uniquify_path(self.default_directory, + fd, fspath = uniquify_path(self.default_directory, '_'.join(basename)) if fspath is None: msg = entity._cw._('failed to uniquify path (%s, %s)') % ( self.default_directory, '_'.join(basename)) raise ValidationError(entity.eid, {role_name(attr, 'subject'): msg}) - return fspath + return fd, fspath def current_fs_path(self, entity, attr): """return the current fs_path of the attribute, or None is the attr is diff -r f3dec653a27d -r 76f601a90aa7 server/test/unittest_storage.py --- a/server/test/unittest_storage.py Thu Jun 11 09:51:30 2015 +0200 +++ b/server/test/unittest_storage.py Mon Jun 23 16:54:35 2014 +0200 @@ -20,6 +20,7 @@ from logilab.common.testlib import unittest_main, tag, Tags from cubicweb.devtools.testlib import CubicWebTC +from glob import glob import os import os.path as osp import shutil @@ -88,16 +89,21 @@ def test_bfss_storage(self): with self.admin_access.repo_cnx() as cnx: f1 = self.create_file(cnx) - expected_filepath = osp.join(self.tempdir, '%s_data_%s' % - (f1.eid, f1.data_name)) - self.assertTrue(osp.isfile(expected_filepath)) + filepaths = glob(osp.join(self.tempdir, '%s_data_*' % f1.eid)) + self.assertEqual(len(filepaths), 1, filepaths) + expected_filepath = filepaths[0] # file should be read only self.assertFalse(os.access(expected_filepath, os.W_OK)) self.assertEqual(file(expected_filepath).read(), 'the-data') cnx.rollback() self.assertFalse(osp.isfile(expected_filepath)) + filepaths = glob(osp.join(self.tempdir, '%s_data_*' % f1.eid)) + self.assertEqual(len(filepaths), 0, filepaths) f1 = self.create_file(cnx) cnx.commit() + filepaths = glob(osp.join(self.tempdir, '%s_data_*' % f1.eid)) + self.assertEqual(len(filepaths), 1, filepaths) + expected_filepath = filepaths[0] self.assertEqual(file(expected_filepath).read(), 'the-data') f1.cw_set(data=Binary('the new data')) cnx.rollback() @@ -114,7 +120,9 @@ with self.admin_access.repo_cnx() as cnx: f1 = self.create_file(cnx) expected_filepath = osp.join(self.tempdir, '%s_data_%s' % (f1.eid, f1.data_name)) - self.assertEqual(self.fspath(cnx, f1), expected_filepath) + base, ext = osp.splitext(expected_filepath) + self.assertTrue(self.fspath(cnx, f1).startswith(base)) + self.assertTrue(self.fspath(cnx, f1).endswith(ext)) def test_bfss_fs_importing_doesnt_touch_path(self): with self.admin_access.repo_cnx() as cnx: