LGTM, thanks.
On Thu, Dec 5, 2013 at 10:48 AM, Santi Raffa <[email protected]> wrote: > On Thu, Dec 5, 2013 at 9:54 AM, Thomas Thrainer <[email protected]> > wrote: > > On Wed, Dec 4, 2013 at 2:50 PM, Santi Raffa <[email protected]> wrote: > >> + def __init__(self, unique_id, children, size, params, dyn_params, > >> + _file_helper_obj=None): > > _file_helper_obj should nowhere be used, so remove the parameter. > > aaaargh! As you might've noticed by now, the fix for this went in the > wrong patch. I knew this would happen (this is also why I ran > commit-check on every single commit) :) > > >> + del self.file > > Why the del here? > > Because I had the wrong mental model of what "del" does. > > >> + self.file = None > >> + self.dev_path = None > >> + self.attached = False > >> + > >> + def Open(self, force=False): > >> + """Make the device ready for I/O. > >> + > >> + This is a no-op for the file type. > >> + > >> + """ > >> + assert self.attached, "Gluster file opened without being attached" > > > > > > self.attached is only ever assigned False, so I wonder how this should > ever > > work. > > This is another one that only exists in this commit but is fixed in > (Gluster: use ssconf value). > > Hence, this is the current interdiff: > > diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py > index fa1532b..64b0d62 100644 > --- a/lib/storage/gluster.py > +++ b/lib/storage/gluster.py > @@ -262,7 +262,7 @@ class GlusterVolume(object): > > > class GlusterStorage(base.BlockDev): > - """File device. > + """File device using the Gluster backend. > > This class represents a file storage backend device stored on Gluster. > Ganeti > mounts and unmounts the Gluster devices automatically. > @@ -311,7 +311,6 @@ class GlusterStorage(base.BlockDev): > > """ > > - del self.file > self.file = None > self.dev_path = None > self.attached = False > > > > ...and this is the (separate) fixup fixing just this commit: > > diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py > index 6cfdbee..9ecb3d1 100644 > --- a/lib/storage/gluster.py > +++ b/lib/storage/gluster.py > @@ -41,8 +41,7 @@ class GlusterStorage(base.BlockDev): > The unique_id for the file device is a (file_driver, file_path) tuple. > > """ > - def __init__(self, unique_id, children, size, params, dyn_params, > - _file_helper_obj=None): > + def __init__(self, unique_id, children, size, params, dyn_params): > """Initalizes a file device backend. > > """ > @@ -130,7 +129,8 @@ class GlusterStorage(base.BlockDev): > @return: True if file exists > > """ > - return self.file.Exists() > + self.attached = self.file.Exists() > + return self.attached > > def GetActualSize(self): > """Return the actual disk size. > @@ -159,6 +159,5 @@ class GlusterStorage(base.BlockDev): > > dev_path = unique_id[1] > > - file_helper = FileDeviceHelper.Create(dev_path, size) > - return GlusterStorage(unique_id, children, size, params, dyn_params, > - _file_helper_obj=file_helper) > + FileDeviceHelper.Create(dev_path, size) > + return GlusterStorage(unique_id, children, size, params, dyn_params) > > > > ...which does not apply cleanly, so here's the diff before and after > this change for "Gluster: mount automatically" too: > > diff --git a/lib/storage/gluster.py b/lib/storage/gluster.py > index 2751d4b..2314538 100644 > --- a/lib/storage/gluster.py > +++ b/lib/storage/gluster.py > @@ -382,8 +382,8 @@ class GlusterStorage(base.BlockDev): > self.volume.Unmount() > raise err > > - self.attached = True > - return self.file.Exists() > + self.attached = self.file.Exists() > + return self.attached > > def GetActualSize(self): > """Return the actual disk size. > > > > -- > 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
