Change in vdsm[master]: sp: Nest try-finally for temporary secure change - vdsm-patches - Fedora mailing-lists

31 Jan 2014

Nir Soffer has uploaded a new change for review.
Change subject: sp: Nest try-finally for temporary secure change
......................................................................
sp: Nest try-finally for temporary secure change
There were two instances where the same try-finally block was used for
both acquiring a lock and setting the pool as secure for a block of
code:
acquire lock
     try:
         set secure
         code that needs both lock and secure state
     finally:
         set unsecure
         release lock
This type of usage is incorrect. try-finally block should be used for
one change to ensure that the change is finally reverted, and to make
the intention of the code clear.
This patch use nested try-finally blocks, one for the lock, and one for
the secure state:
acquire lock
     try:
         set secure
         try:
             code that needs both lock and secure state
         finally:
             set unsecure
     finally:
         release lock
Because of the extra nesting, comments inside the nested code were
reformatted and missing punctuation was added.
I considered replacing the nested try-finally with a secured context
manager, but since the current code uses try-except-finally, using a
context manager would create an even deeper nesting or extracting some
new private methods. To keep minimal changes, I avoid this direction.
It seems that the secure state block can become smaller - some method
inside the secured block are @unsecured. I kept the secure block
semantics are they are now, and will check minimizing the secured block
in a later patch.
Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b
Signed-off-by: Nir Soffer nsoffer@redhat.com
---
M vdsm/storage/sp.py
1 file changed, 48 insertions(+), 48 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/23955/1

diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index a020e1e..c26f7a8 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -566,40 +566,41 @@
fileUtils.createdir(self.poolPath)
         self._acquireTemporaryClusterLock(msdUUID, leaseParams)
-
         try:
             self._setSecure()
-            # Mark 'master' domain
-            # We should do it before actually attaching this domain to the pool
-            # During 'master' marking we create pool metadata and each attached
-            # domain should register there
-            self.createMaster(poolName, msd, masterVersion, leaseParams)
-            self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
-            # Attach storage domains to the storage pool
-            # Since we are creating the pool then attach is done from the hsm
-            # and not the spm therefore we must manually take the master domain
-            # lock
-            # TBD: create will receive only master domain and further attaches
-            #      should be done under SPM
-
-            # Master domain was already attached (in createMaster),
-            # no need to reattach
-            for sdUUID in domList:
-                # No need to attach the master
-                if sdUUID != msdUUID:
-                    self.attachSD(sdUUID)
-        except Exception:
-            self.log.error("Create pool %s canceled ", poolName, exc_info=True)
             try:
-                fileUtils.cleanupdir(self.poolPath)
-                self.__cleanupDomains(domList, msdUUID, masterVersion)
-            except:
-                self.log.error("Cleanup failed due to an unexpected error",
-                               exc_info=True)
-            raise
-        finally:
-            self._setUnsecure()
+                # Mark 'master' domain.  We should do it before actually
+                # attaching this domain to the pool During 'master' marking we
+                # create pool metadata and each attached domain should register
+                # there.
+                self.createMaster(poolName, msd, masterVersion, leaseParams)
+                self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
+                # Attach storage domains to the storage pool.  Since we are
+                # creating the pool then attach is done from the hsm and not
+                # the spm therefore we must manually take the master domain
+                # lock.
+                # TBD: create will receive only master domain and further
+                #      attaches should be done under SPM.
+                # Master domain was already attached (in createMaster), no need
+                # to reattach.
+                for sdUUID in domList:
+                    # No need to attach the master
+                    if sdUUID != msdUUID:
+                        self.attachSD(sdUUID)
+            except Exception:
+                self.log.error("Create pool %s canceled ", poolName,
+                               exc_info=True)
+                try:
+                    fileUtils.cleanupdir(self.poolPath)
+                    self.__cleanupDomains(domList, msdUUID, masterVersion)
+                except:
+                    self.log.error("Cleanup failed due to an unexpected error",
+                                   exc_info=True)
+                raise
+            finally:
+                self._setUnsecure()
+        finally:
             self._releaseTemporaryClusterLock(msdUUID)
             self.stopMonitoringDomains()
@@ -697,29 +698,28 @@
             # The host id must be set for createMaster(...).
             self.id = hostId
             temporaryLock = False
-
-        # As in the create method we need to temporarily set the object
-        # secure in order to change the domains map.
-        # TODO: it is clear that reconstructMaster and create (StoragePool)
-        # are extremely similar and they should be unified.
-        self._setSecure()
-
         try:
-            self.createMaster(poolName, futureMaster, masterVersion,
-                              leaseParams)
-            self.setMasterDomain(msdUUID, masterVersion)
+            # As in the create method we need to temporarily set the object
+            # secure in order to change the domains map.
+            # TODO: it is clear that reconstructMaster and create (StoragePool)
+            # are extremely similar and they should be unified.
+            self._setSecure()
+            try:
+                self.createMaster(poolName, futureMaster, masterVersion,
+                                  leaseParams)
+                self.setMasterDomain(msdUUID, masterVersion)
-            for sdUUID in domDict:
-                domDict[sdUUID] = domDict[sdUUID].capitalize()
+                for sdUUID in domDict:
+                    domDict[sdUUID] = domDict[sdUUID].capitalize()
-            # Add domain to domain list in pool metadata.
-            self.log.info("Set storage pool domains: %s", domDict)
-            self._backend.setDomainsMap(domDict)
+                # Add domain to domain list in pool metadata.
+                self.log.info("Set storage pool domains: %s", domDict)
+                self._backend.setDomainsMap(domDict)
-            self.refresh(msdUUID=msdUUID, masterVersion=masterVersion)
+                self.refresh(msdUUID=msdUUID, masterVersion=masterVersion)
+            finally:
+                self._setUnsecure()
         finally:
-            self._setUnsecure()
-
             if temporaryLock:
                 self._releaseTemporaryClusterLock(msdUUID)
                 self.stopMonitoringDomains()
-- 
To view, visit http://gerrit.ovirt.org/23955
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer nsoffer@redhat.com

    