On Wed, Dec 4, 2013 at 2:50 PM, Santi Raffa <[email protected]> wrote:
> This will allow code reuse for Gluster through composition, rather > than inheritance. > > Signed-off-by: Santi Raffa <[email protected]> > --- > lib/storage/filestorage.py | 159 > ++++++++++++++++++------- > test/py/ganeti.storage.filestorage_unittest.py | 72 +++++++++++ > 2 files changed, 190 insertions(+), 41 deletions(-) > > diff --git a/lib/storage/filestorage.py b/lib/storage/filestorage.py > index ddca056..d544363 100644 > --- a/lib/storage/filestorage.py > +++ b/lib/storage/filestorage.py > @@ -32,9 +32,120 @@ from ganeti import constants > from ganeti import errors > from ganeti import pathutils > from ganeti import utils > +from ganeti.utils import io > from ganeti.storage import base > > > +class FileDeviceHelper(object): > + > + @classmethod > + def Create(cls, path, size, create_folders=False, > + _file_path_acceptance_fn=None): > CreateFile would be a better name, as that's what is being created primarily. > + """Create a new file and its file device helper. > + > + @param size: the size in MiBs the file should be truncated to. > + @param create_folders: create the directories for the path if > necessary > + (using L{ganeti.utils.io.Makedirs}) > + @raise errors.FileStoragePathError: if the file path is disallowed by > policy > + > + """ > + > + if create_folders: > + folder = os.path.dirname(path) > + io.Makedirs(folder) > + > + try: > + fd = os.open(path, os.O_RDWR | os.O_CREAT | os.O_EXCL) > + f = os.fdopen(fd, "w") > + f.truncate(size * 1024 * 1024) > + f.close() > + except EnvironmentError as err: > + base.ThrowError("%s: can't create: %s", path, str(err)) > + > + return FileDeviceHelper(path, > + > _file_path_acceptance_fn=_file_path_acceptance_fn) > Why return a FileDeviceHelper here? The return value is nowhere actually used, right? And if the method is named CreateFile, returning a FileDeviceHelper would be weird anyway... > + > + def __init__(self, path, > + _file_path_acceptance_fn=None): > + """Create a new file device helper. > + > + @raise errors.FileStoragePathError: if the file path is disallowed by > policy > + > + """ > + if not _file_path_acceptance_fn: > + _file_path_acceptance_fn = CheckFileStoragePathAcceptance > + > + _file_path_acceptance_fn(path) > I would perform this check in Create() and don't care about this in __init__. This way it's easier to test this class, and it's enough to verify the right path when we're creating the file, right? Additionally, there's a security risk if we do it this way, as _first_ we create the file (potentially overwriting something), and only then we _check_ if the path was OK. > + self.path = path > + > + def Exists(self, assert_exists=None): > + """Check for the existence of the given file. > + > + @param assert_exists: creates an assertion on the result value: > + - if true, raise errors.BlockDeviceError if the file doesn't exist > + - if false, raise errors.BlockDeviceError if the file does exist > + @rtype: boolean > + @return: True if the file exists > + > + """ > + > + exists = os.path.isfile(self.path) > + > + if not exists and assert_exists is True: > + raise base.ThrowError("%s: No such file", self.path) > + if exists and assert_exists is False: > + raise base.ThrowError("%s: File exists", self.path) > + > + return exists > + > + def Remove(self): > + """Remove the file backing the block device. > + > + @rtype: boolean > + @return: True if the removal was successful > + > + """ > + try: > + os.remove(self.path) > + return True > + except OSError as err: > + if err.errno != errno.ENOENT: > + base.ThrowError("%s: can't remove: %s", self.path, err) > + return False > + > + def Size(self): > + """Return the actual disk size in bytes. > + > + @rtype: int > + @return: The file size in bytes. > + > + """ > + self.Exists(assert_exists=True) > + try: > + return os.stat(self.path).st_size > + except OSError as err: > + base.ThrowError("%s: can't stat: %s", self.path, err) > + > + def Grow(self, amount): > + """Grow the file > + > + @param amount: the amount (in mebibytes) to grow by. > + > + """ > + # Check that the file exists > + self.Exists(assert_exists=True) > + current_size = self.Size() > + if amount < 0: > + base.ThrowError("%s: can't grow by negative amount", self.path) > + new_size = current_size + amount * 1024 * 1024 > + try: > + f = open(self.path, "a+") > + f.truncate(new_size) > + f.close() > + except EnvironmentError, err: > + base.ThrowError("%s: can't grow: ", self.path, str(err)) > + > + > class FileStorage(base.BlockDev): > """File device. > > @@ -55,9 +166,7 @@ class FileStorage(base.BlockDev): > raise ValueError("Invalid configuration data %s" % str(unique_id)) > self.driver = unique_id[0] > self.dev_path = unique_id[1] > - > - CheckFileStoragePathAcceptance(self.dev_path) > - > + self.file = FileDeviceHelper(self.dev_path) > self.Attach() > > def Assemble(self): > @@ -66,8 +175,7 @@ class FileStorage(base.BlockDev): > Checks whether the file device exists, raises BlockDeviceError > otherwise. > > """ > - if not os.path.exists(self.dev_path): > - base.ThrowError("File device '%s' does not exist" % self.dev_path) > + self.file.Exists(assert_exists=True) > > def Shutdown(self): > """Shutdown the device. > @@ -101,11 +209,7 @@ class FileStorage(base.BlockDev): > @return: True if the removal was successful > > """ > - try: > - os.remove(self.dev_path) > - except OSError, err: > - if err.errno != errno.ENOENT: > - base.ThrowError("Can't remove file '%s': %s", self.dev_path, err) > + return self.file.Remove() > > def Rename(self, new_id): > """Renames the file. > @@ -122,20 +226,9 @@ class FileStorage(base.BlockDev): > """ > if not backingstore: > return > - # Check that the file exists > - self.Assemble() > - current_size = self.GetActualSize() > - new_size = current_size + amount * 1024 * 1024 > - assert new_size > current_size, "Cannot Grow with a negative amount" > - # We can't really simulate the growth > if dryrun: > return > - try: > - f = open(self.dev_path, "a+") > - f.truncate(new_size) > - f.close() > - except EnvironmentError, err: > - base.ThrowError("Error in file growth: %", str(err)) > + self.file.Grow(amount) > As discussed previously, it would be better to pass (at least) dryrun to self.file.Grow(..). Right now, we loose the check for file existence and also the assertion which was in place previously. With this patch, no checks are performed in the dry_run case. As those checks belong to FileDeviceHelper, it'd be better to pass in at least this flag and let the FileDeviceHelper do the checks in a general way. > > def Attach(self): > """Attach to an existing file. > @@ -146,7 +239,7 @@ class FileStorage(base.BlockDev): > @return: True if file exists > > """ > - self.attached = os.path.exists(self.dev_path) > + self.attached = self.file.Exists() > return self.attached > > def GetActualSize(self): > @@ -155,12 +248,7 @@ class FileStorage(base.BlockDev): > @note: the device needs to be active when this is called > > """ > - assert self.attached, "BlockDevice not attached in GetActualSize()" > - try: > - st = os.stat(self.dev_path) > - return st.st_size > - except OSError, err: > - base.ThrowError("Can't stat %s: %s", self.dev_path, err) > + return self.file.Size() > > @classmethod > def Create(cls, unique_id, children, size, spindles, params, excl_stor, > @@ -182,18 +270,7 @@ class FileStorage(base.BlockDev): > > dev_path = unique_id[1] > > - CheckFileStoragePathAcceptance(dev_path) > - > - try: > - fd = os.open(dev_path, os.O_RDWR | os.O_CREAT | os.O_EXCL) > - f = os.fdopen(fd, "w") > - f.truncate(size * 1024 * 1024) > - f.close() > - except EnvironmentError, err: > - if err.errno == errno.EEXIST: > - base.ThrowError("File already existing: %s", dev_path) > - base.ThrowError("Error in file creation: %", str(err)) > - > + FileDeviceHelper.Create(dev_path, size) > return FileStorage(unique_id, children, size, params, dyn_params) > > > diff --git a/test/py/ganeti.storage.filestorage_unittest.py b/test/py/ > ganeti.storage.filestorage_unittest.py > index 43b8de3..cb34c3d 100755 > --- a/test/py/ganeti.storage.filestorage_unittest.py > +++ b/test/py/ganeti.storage.filestorage_unittest.py > @@ -28,7 +28,9 @@ import unittest > > from ganeti import errors > from ganeti.storage import filestorage > +from ganeti.utils import io > from ganeti import utils > +from ganeti import constants > > import testutils > > @@ -219,5 +221,75 @@ class > TestCheckFileStoragePathExistance(testutils.GanetiTestCase): > "/usr/lib64/xyz", _filename=tmpfile) > > > +class TestFileDeviceHelper(testutils.GanetiTestCase): > + > + @staticmethod > + def _Make(path, create_with_size=None, create_folders=False): > + skip_checks = lambda path: None > + if create_with_size: > + return filestorage.FileDeviceHelper.Create( > + path, create_with_size, create_folders=create_folders, > + _file_path_acceptance_fn=skip_checks > + ) > + else: > + return filestorage.FileDeviceHelper(path, > + > _file_path_acceptance_fn=skip_checks) > + > + > + def test(self): > Didn't we agree on splitting this test method into a bunch of smaller ones with the common initialization logic in a setUp() method? > + # Get temp directory > + directory = tempfile.mkdtemp() > + subdirectory = io.PathJoin(directory, "pinky") > + path = io.PathJoin(subdirectory, "bunny") > + > + should_fail = lambda fn: self.assertRaises(errors.BlockDeviceError, > fn) > + > + # Make sure it doesn't exist, and methods check for it > + TestFileDeviceHelper._Make(path).Exists(assert_exists=False) > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path).Exists(assert_exists=True)) > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path).Size()) > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path).Grow(20)) > + > + # Removing however fails silently. > + TestFileDeviceHelper._Make(path).Remove() > + > + # Make sure we don't create all directories for you unless we ask for > it > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path, create_with_size=42)) > + > + # Create the file. > + TestFileDeviceHelper._Make(path, create_with_size=42, > create_folders=True) > + > + # This should still fail. > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(subdirectory).Size()) > + > + > + self.assertTrue(TestFileDeviceHelper._Make(path).Exists()) > + > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path, create_with_size=42)) > + > + TestFileDeviceHelper._Make(path).Exists(assert_exists=True) > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path).Exists(assert_exists=False)) > + > + should_fail( lambda: \ > + TestFileDeviceHelper._Make(path).Grow(-30)) > + > + TestFileDeviceHelper._Make(path).Grow(58) > + self.assertEqual(100 * 1024 * 1024, > + TestFileDeviceHelper._Make(path).Size()) > + > + TestFileDeviceHelper._Make(path).Remove() > + TestFileDeviceHelper._Make(path).Exists(assert_exists=False) > + > + os.rmdir(subdirectory) > + os.rmdir(directory) > + > + > if __name__ == "__main__": > testutils.GanetiTestProgram() > -- > 1.8.4.1 > > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
