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 <[email protected]>
---
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 <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
