Timothy Asir has posted comments on this change. Change subject: gluster: Added gluster hooks support ......................................................................
Patch Set 18: No score (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() This is used more frequently in hooksList function to get the mime type for every hook file in the hook directory. Loading this for every iteration might causes an performance issue. So it is required to load at the module initialization time. 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): Done 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: Unfortunately we can't avoid path.check here. Because, this function is used by hook add and hook update. During 'hook update', it will raise an error on hook found and during 'hook add' it raises when a hook file is not found. 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: Yes, you have raised a valid point here! However, Its required to check the availability of the file before deleting. Because OSError can occur for some other reason also (other than file not found). In that case we may missed the actual error. For example: enabledFile has found with some permission issue or any other issues. We may miss the actual error and will end-up saying disableFile not found or hook file not found. I will update this function in such a way that, it will delete the enabledFile and disabledFile only if its exists and raise appropriate OSError. 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
