On Mon, Dec 9, 2013 at 11:03 AM, Santi Raffa <[email protected]> wrote:
> On Thu, Dec 5, 2013 at 1:41 PM, Thomas Thrainer <[email protected]> > wrote: > > True, there you get a fobj with which you are doing something. IMHO, this > > makes it more obvious to the reader that the with actually did something > and > > returned something to you. > > Also, the more prominent example is 'with open(...) as f', where it's > > immediately obvious that an 'open' call is issued. In fact, the 'with > > file(...) as fobj' form is even discouraged to use in the Python docs > > (http://docs.python.org/2/library/functions.html?highlight=file#file). > > > > 'with self.volume' doesn't tell the reader, who doesn't know the > internals > > of the GlusterVolume class, what's happening. That's why I proposed to > > change it to 'with self.volume.Mount()', which reads much better IMHO > (and > > it's not so hard to return a small context object in Mount which unmounts > > once going out of the context). > > Okay. I've already merged the changes in my local branch but these > should be the relevant interdiffs: > > diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py > index 4e253de..8e588de 100644 > --- a/lib/storage/gluster.py > +++ b/lib/storage/gluster.py > @@ -202,10 +202,32 @@ class GlusterVolume(object): > """Try and mount the volume. No-op if the volume is already mounted. > > @raises BlockDeviceError: if the mount was unsuccessful > + > + @rtype: context manager > + @return: A simple context monager that lets you use this volume for > s/monagar/manager/ > + short lived operations like so:: > + > + with volume.mount(): > + # Do operations on volume > + # Volume is now unmounted > + > """ > > + class _GlusterVolumeContextManager(object): > + > + def __init__(self, volume): > + self.volume = volume > + > + def __enter__(self): > + # We're already mounted. > + return self > + > + def __exit__(self, *exception_information): > + self.volume.Unmount() > + return False # do not swallow exceptions. > + > if self._IsMounted(): > - return > + return _GlusterVolumeContextManager(self) > > command = ["mount", > "-t", "glusterfs", > @@ -233,6 +255,8 @@ class GlusterVolume(object): > self.mount_point, > reasons) > > + return _GlusterVolumeContextManager(self) > + > def Unmount(self): > """Try and unmount the volume. > > @@ -251,13 +275,6 @@ class GlusterVolume(object): > logging.warning("Failed to unmount %r from %r: %s", > self, self.mount_point, result.fail_reason) > > - def __enter__(self): > - self.Mount() > - return self > - > - def __exit__(self, *exception_information): > - self.Unmount() > - > > class GlusterStorage(base.BlockDev): > """File device using the Gluster backend. > @@ -335,7 +352,7 @@ class GlusterStorage(base.BlockDev): > @return: True if the removal was successful > > """ > - with self.volume: > + with self.volume.Mount(): > self.file = FileDeviceHelper(self.full_path) > if self.file.Remove(): > self.file = None > @@ -427,7 +444,7 @@ 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: > + with volume_obj.Mount(): > FileDeviceHelper.CreateFile(full_path, size, create_folders=True) > > return GlusterStorage(unique_id, children, size, params, dyn_params) > > > Rest LGTM, thanks. > -- > 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
