Aravinda VK has posted comments on this change.
Change subject: gluster: Added gluster hooks support
......................................................................
Patch Set 18: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/gluster/hooks.py
Line 29: from . import safeWrite
Line 30:
Line 31: _glusterHooksPath = '/var/lib/glusterd/hooks/1'
Line 32: _mimeType = magic.open(magic.MIME_TYPE)
Line 33: _mimeType.load()
Move above two lines to a function and use it whenever required. You can have
function _getMimeType(fileName)
Problem is
1) This is used in HooksRead and HooksList only. other functionality not uses
this.
2) Since this is using ctypes, _mimeType.load() is loading shared object
"magic" which is overhead for other verbs.
Line 34:
Line 35:
Line 36: class HookLevel:
Line 37: PRE = 'PRE'
Line 102: return hooks
Line 103:
Line 104: hooks = []
Line 105: try:
Line 106: for gCmd in os.listdir(_glusterHooksPath):
Raises exception if hooks dir has any files(other than directory). We can
ignore if any files present. Add if os.path.isdir(gCmd) check
Line 107: hooks += _getHooks(gCmd, HookLevel.PRE)
Line 108: hooks += _getHooks(gCmd, HookLevel.POST)
Line 109: return hooks
Line 110: except OSError, e:
Line 195: update=True, enable=False):
Line 196: enabledFile, disabledFile = _getHookFileNames(glusterCmd,
Line 197: hookLevel.lower(),
hookName)
Line 198: hookFound = os.path.exists(enabledFile) or
os.path.exists(disabledFile)
Line 199: if update:
Avoid path.exists check here. handle exception while writing.
Line 200: if not hookFound:
Line 201: raise ge.GlusterHookNotFoundException(glusterCmd,
hookLevel,
Line 202: hookName)
Line 203: else:
Line 243: @checkArgs
Line 244: @exportToSuperVdsm
Line 245: def hookRemove(glusterCmd, hookLevel, hookName):
Line 246: hookFile = _getHookFile(glusterCmd, hookLevel.lower(), hookName)
Line 247: try:
Instead of getting hookFile, get enabledFile and disabledFile from
_getHookFileNames, Then try deleting enabledFile if failed try deleting
disabledFile.
try:
os.remove(enabledFile)
except OSError:
os.remove(disabledFile)
except OSError:
# raise exception part
Advantage:
Multiple path.exists checks can be reduced.
Even after checking path.exists what if file gets deleted or renamed in backend?
Line 248: os.remove(hookFile)
Line 249: return True
Line 250: except OSError, e:
Line 251: errMsg = "[Errno %s] %s: '%s'" % (e.errno, e.strerror,
e.filename)
--
To view, visit http://gerrit.ovirt.org/9671
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3918aa035d90967f1297dc7fadcf14b6a9385c45
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir <[email protected]>
Gerrit-Reviewer: Aravinda VK <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Bala.FA <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Timothy Asir <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
