On Thu, Dec 5, 2013 at 2:31 PM, Santi Raffa <[email protected]> wrote:
> >> class FileDeviceHelper(object):
> >> + """Model a file in a file system for use as instance storage.
> >> +
> >> + """
> >
> >
> > FileDeviceHelper is what it says it is: a helper class dealing with file
> > devices. If it was modeling a file, the class would be named File,
> right? So
> > I don't quite like this misleading comment...
>
> Okay, I'll drop the docstring then.
>
> >> - def Grow(self, amount):
> >> + def Grow(self, amount, dryrun, backingstore, excl_stor):
> >
> > Why excl_stor?
>
> I figured that if we wanted to merge the method implementation between
> the FileStorage and GlusterStorage methods, including the handling of
> additional parameters, then it made sense to pass *all* parameters to
> the Helper even if excl_storage is ignored.
>
> Second interdiff, also to be split in two fixups:
>
> diff --git a/lib/storage/filestorage.py b/lib/storage/filestorage.py
> index 185fdf4..1bafef0 100644
> --- a/lib/storage/filestorage.py
> +++ b/lib/storage/filestorage.py
> @@ -37,13 +37,10 @@ from ganeti.storage import base
>
>
> class FileDeviceHelper(object):
> - """Model a file in a file system for use as instance storage.
> -
> - """
>
> @classmethod
> - def Create(cls, path, size, create_folders=False,
> - _file_path_acceptance_fn=None):
> + def CreateFile(cls, path, size, create_folders=False,
> + _file_path_acceptance_fn=CheckFileStoragePathAcceptance):
> """Create a new file and its file device helper.
>
> @param size: the size in MiBs the file should be truncated to.
> @@ -56,8 +53,6 @@ class FileDeviceHelper(object):
>
> """
>
> - if not _file_path_acceptance_fn:
> - _file_path_acceptance_fn = CheckFileStoragePathAcceptance
> _file_path_acceptance_fn(path)
>
> if create_folders:
> @@ -76,14 +71,12 @@ class FileDeviceHelper(object):
>
> _file_path_acceptance_fn=_file_path_acceptance_fn)
>
> def __init__(self, path,
> - _file_path_acceptance_fn=None):
> + _file_path_acceptance_fn=_file_path_acceptance_fn):
>
You meant CheckFileStoragePathAcceptance as default, right?
> """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)
>
> self.path = path
> @@ -145,14 +138,15 @@ class FileDeviceHelper(object):
> # Check that the file exists
> self.Exists(assert_exists=True)
>
> + if amount < 0:
> + base.ThrowError("%s: can't grow by negative amount", self.path)
> +
> if not backingstore:
> return
> if dryrun:
> return
>
> 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+")
> @@ -286,7 +280,7 @@ class FileStorage(base.BlockDev):
>
> dev_path = unique_id[1]
>
> - FileDeviceHelper.Create(dev_path, size)
> + FileDeviceHelper.CreateFile(dev_path, size)
> return FileStorage(unique_id, children, size, params, dyn_params)
>
>
> diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py
> index 9ce76c3..d4f9650 100644
> --- a/lib/storage/gluster.py
> +++ b/lib/storage/gluster.py
> @@ -431,6 +431,6 @@ class GlusterStorage(base.BlockDev):
> # Possible optimization: defer actual creation to first Attach, rather
> # than mounting and unmounting here, then remounting immediately
> after.
> with volume_obj:
> - FileDeviceHelper.Create(full_path, size, create_folders=True)
> + FileDeviceHelper.CreateFile(full_path, size, create_folders=True)
>
> return GlusterStorage(unique_id, children, size, params, dyn_params)
>
>
> --
> Raffa Santi
> 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
>
--
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
