Nir Soffer has posted comments on this change. Change subject: storage: Add logging on filesystem altering operations ......................................................................
Patch Set 5: Code-Review-1 (4 comments) Mostly nice, but there are some place which duplicated some logic for the log, which make the code worse. http://gerrit.ovirt.org/#/c/26046/5/vdsm/storage/fileSD.py File vdsm/storage/fileSD.py: Line 391: try: Line 392: self.log.debug("Removing file: %s", volPath) Line 393: self.oop.os.remove(volPath) Line 394: self.log.debug("Removing file: %s", volPath + '.meta') Line 395: self.oop.os.remove(volPath + '.meta') There is duplication here - better: metafile = volPath + '.meta' self.log.debug("Removing file: %s", metaPath) self.oop.os.remove(metaPath) Line 396: self.log.debug("Removing file: %s", volPath + '.lease') Line 397: self.oop.os.remove(volPath + '.lease') Line 398: except OSError: Line 399: self.log.error("vol: %s can't be removed.", Line 397: self.oop.os.remove(volPath + '.lease') Line 398: except OSError: Line 399: self.log.error("vol: %s can't be removed.", Line 400: volPath, exc_info=True) Line 401: try: Well if we log all operations, why not log this one also? Line 402: self.oop.os.rmdir(toDelDir) Line 403: except OSError as e: Line 404: self.log.error("removed image dir: %s can't be removed", toDelDir) Line 405: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) http://gerrit.ovirt.org/#/c/26046/5/vdsm/storage/fileUtils.py File vdsm/storage/fileUtils.py: Line 380: if (mode & 0o070) != newGroupMode: # group mode mask Line 381: # setting the new group mode masking out the original one Line 382: log.debug("Changing mode for %s to %#o", path, Line 383: (mode & 0o707) | newGroupMode) Line 384: os.chmod(path, (mode & 0o707) | newGroupMode) The logic for the new mode is duplicated, which will lead to mismatch when someone change the value in the chmod call but forget the log. Better: newMode = (mode & 0o707) | newGroupMode log it... chmod... Line 385: Line 386: Line 387: def padToBlockSize(path): Line 388: with file(path, 'a') as f: Line 388: with file(path, 'a') as f: Line 389: size = os.fstat(f.fileno()).st_size Line 390: log.debug("Truncating file %s to %s", path, Line 391: 512 * ((size + 511) / 512)) Line 392: os.ftruncate(f.fileno(), 512 * ((size + 511) / 512)) Same duplication issue -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Xavi Francisco <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[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
