Nir Soffer has posted comments on this change.
Change subject: securable: refactor the scurable implementation
......................................................................
Patch Set 2:
(6 comments)
Can you explain what do we need this strange concept of "safe" or "secured"
class/methods?
....................................................
File tests/main.py
Line 25: from storage import storage_exception
Line 26: from vdsm.utils import GeneralException
Line 27:
Line 28: from testrunner import VdsmTestCase as TestCaseBase
Line 29:
Where are the new tests?
Line 30:
Line 31: class TestStorageExceptions(TestCaseBase):
Line 32: def test_collisions(self):
Line 33: codes = {}
....................................................
File vdsm/storage/securable.py
Line 21: from functools import update_wrapper
Line 22:
Line 23: OVERRIDE_ARG = "__securityOverride"
Line 24: SECURE_FIELD = "__secured__"
Line 25: SECURE_METHOD = 'isSafe'
This has to match method implemented by the "safe" class.
Line 26:
Line 27:
Line 28: class SecureError(RuntimeError):
Line 29: pass
Line 55: raise SecureError("Secured object is not in safe state")
Line 56:
Line 57: return method(self, *args, **kwargs)
Line 58:
Line 59: return update_wrapper(wrapper, method)
Why update_wrapper is better then @wraps?
....................................................
File vdsm/storage/sp.py
Line 42: from sdc import sdCache
Line 43: import storage_exception as se
Line 44: from persistentDict import DictValidator, unicodeEncoder,
unicodeDecoder
Line 45: from remoteFileHandler import Timeout
Line 46: from securable import secured, unsecured
These names are confusing - secured is class decorator, and unsecured is
function decorator, but the name suggest that they are the same type.
Line 47: import image
Line 48: from resourceFactories import IMAGE_NAMESPACE
Line 49: from storageConstants import STORAGE
Line 50: import resourceManager as rm
Line 131: self.domainMonitor = domainMonitor
Line 132: self._upgradeCallback =
partial(StoragePool._upgradePoolDomain,
Line 133: proxy(self))
Line 134:
Line 135: def isSafe(self):
How users of the secured class decorator can know that they must implement
"isSafe" method?
Also "security" and "safety" are different concepts. Lets choose one of them.
Line 136: return self._safety.isSet()
Line 137:
Line 138: @unsecured
Line 139: def getSpmLver(self):
Line 319:
Line 320: self.spmRole = SPM_ACQUIRED
Line 321:
Line 322: # Once setSafe completes we are running as SPM
Line 323: self._safety.set()
This is worse then the original version. I would add a "_beSafe" and
"_beUnsafe" methods.
Line 324: self._updateDomainsRole()
Line 325:
Line 326: # Mailbox issues SPM commands, therefore we start it
AFTER spm
Line 327: # commands are allowed to run to prevent a race
between the
--
To view, visit http://gerrit.ovirt.org/22115
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
