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