On Thu, Dec 5, 2013 at 10:19 AM, Santi Raffa <[email protected]> wrote:
> On Thu, Dec 5, 2013 at 9:43 AM, Thomas Thrainer <[email protected]> > wrote: > >> + 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. > >> + 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... > > My idea is for the FileDeviceHelper to be essentially an object > representing the file itself in ways that are useful to the disk > template implementations. I think of this method as an alternative > constructor for files that don't yet exist (a "factory" if you will), > and as such I think it should return the "file object" that's been > created. > > That said, no, I can't find a place where the return object is used > now. Please let me know if you still think it should return None and > be named CreateFile instead. > Yes, I do. > > > 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? > > You are right that Create must also check the path, but I'd rather > check it every single time as well in order to honor changes in the > allowed paths file. > > > Didn't we agree on splitting this test method into a bunch of smaller > ones > > with the common initialization logic in a setUp() method? > > Yes; that's coming in at least in a second interdiff. > > ~~~ > > Here's the current interdiff before I administer the changes to the > respective commits. > > diff --git a/lib/storage/filestorage.py b/lib/storage/filestorage.py > index d544363..185fdf4 100644 > --- a/lib/storage/filestorage.py > +++ b/lib/storage/filestorage.py > @@ -19,7 +19,7 @@ > # 02110-1301, USA. > > > -"""Disk-based disk templates and functions. > +"""Filesystem-based access functions and disk templates. > > """ > > @@ -37,6 +37,9 @@ from ganeti.storage import base > > > 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... > > @classmethod > def Create(cls, path, size, create_folders=False, > @@ -46,10 +49,17 @@ class FileDeviceHelper(object): > @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}) > + > + @rtype FileDeviceHelper > + @return The FileDeviceHelper object representing the object. > @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 think it'd be cleaner to put this as the default value in the parameter list. > if create_folders: > folder = os.path.dirname(path) > io.Makedirs(folder) > @@ -74,8 +84,8 @@ class FileDeviceHelper(object): > """ > if not _file_path_acceptance_fn: > _file_path_acceptance_fn = CheckFileStoragePathAcceptance > Here maybe as well, even if we're duplicating the default value. But as it's only changeable for testing purposes, I'd rather opt for the duplicate default value. > - > _file_path_acceptance_fn(path) > + > self.path = path > > def Exists(self, assert_exists=None): > @@ -126,7 +136,7 @@ class FileDeviceHelper(object): > except OSError as err: > base.ThrowError("%s: can't stat: %s", self.path, err) > > - def Grow(self, amount): > + def Grow(self, amount, dryrun, backingstore, excl_stor): > Why excl_stor? > """Grow the file > > @param amount: the amount (in mebibytes) to grow by. > @@ -134,6 +144,12 @@ class FileDeviceHelper(object): > """ > # Check that the file exists > self.Exists(assert_exists=True) > + > + 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) > (At least) this check makes sense for dryrun too. > @@ -228,7 +244,7 @@ class FileStorage(base.BlockDev): > return > if dryrun: > return > - self.file.Grow(amount) > + self.file.Grow(amount, amount, dryrun, backingstore, excl_stor) > Same here? > > def Attach(self): > """Attach to an existing file. > diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py > index 26ab201..fa1532b 100644 > --- a/lib/storage/gluster.py > +++ b/lib/storage/gluster.py > @@ -359,11 +359,7 @@ class GlusterStorage(base.BlockDev): > @param amount: the amount (in mebibytes) to grow with > > """ > - if not backingstore: > - return > - if dryrun: > - return > - self.file.Grow(amount) > + self.file.Grow(amount, dryrun, backingstore, excl_stor) > > def Attach(self): > """Attach to an existing file. > > > -- > 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
