Dan Kenigsberg has posted comments on this change. Change subject: storage: Add logging on filesystem altering operations ......................................................................
Patch Set 8: (2 comments) http://gerrit.ovirt.org/#/c/26046/8/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py: Line 1075: for volUUID in volUUIDs: Line 1076: srcVol = os.path.join(srcImgPath, volUUID) Line 1077: dstVol = os.path.join(imgRunDir, volUUID) Line 1078: try: Line 1079: self.log.debug("Creating symlink from %s to %s", srcVol, try-except blocks should be as short as possible. There's no need to put this log line inside it. If for some reason, self.log.debug() raises an EEXIST, you are going to add a false log line in 1084. So if you can keep as many of your changes out of their respective try-except block, it would be an improvement. Line 1080: dstVol) Line 1081: os.symlink(srcVol, dstVol) Line 1082: except OSError as e: Line 1083: if e.errno == errno.EEXIST: http://gerrit.ovirt.org/#/c/26046/8/vdsm/storage/blockVolume.py File vdsm/storage/blockVolume.py: Line 250: self.log.error("cannot remove volume %s/%s", self.sdUUID, Line 251: self.volUUID, exc_info=True) Line 252: Line 253: try: Line 254: self.log.debug("Unlinking %s", vol_path) here we try hard to swallow exceptions, so it's fine. Line 255: os.unlink(vol_path) Line 256: return True Line 257: except Exception as e: Line 258: eFound = e -- To view, visit http://gerrit.ovirt.org/26046 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3602513af123951f71091c03f799e36ea759aa61 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xavi Francisco <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Xavi Francisco <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
