Nir Soffer has posted comments on this change. Change subject: fileSD: fix deleteImage flow ......................................................................
Patch Set 1: (4 comments) Nice https://gerrit.ovirt.org/#/c/50804/1/vdsm/storage/fileSD.py File vdsm/storage/fileSD.py: Line 223: except OSError as e: Line 224: self.log.error("removed image dir: %s can't be removed", toDelDir) Line 225: raise se.ImageDeleteError("%s %s" % (imgUUID, str(e))) Line 226: Line 227: def _deleteFile(self, fileType, filePath): - Rename _deleteFile to _deleteVolumeFile. This is not a generic utility for deleting files. - There is not need for fileType, the type of the file is clear from the file path. - rename filePath to path. Line 228: try: Line 229: self.oop.os.remove(filePath) Line 230: self.log.debug("%s file: %s was removed.", fileType, filePath) Line 231: except OSError as ose: Line 226: Line 227: def _deleteFile(self, fileType, filePath): Line 228: try: Line 229: self.oop.os.remove(filePath) Line 230: self.log.debug("%s file: %s was removed.", fileType, filePath) Put the log *before* the operation - if the operation get stuck, we want to see this log, so we know what the machine is doing now. Keep the previous log format, it was good: self.log.debug("Removing file: %s", path) Line 231: except OSError as ose: Line 232: if ose.errno != errno.ENOENT: Line 233: self.log.error("%s file: %s can't be removed.", Line 234: fileType, filePath, exc_info=True) Line 227: def _deleteFile(self, fileType, filePath): Line 228: try: Line 229: self.oop.os.remove(filePath) Line 230: self.log.debug("%s file: %s was removed.", fileType, filePath) Line 231: except OSError as ose: Don't use such names, in this context there is only *one* exception, and the best name for it is "e". Even if we had multiple exceptions there is no need to use different exception names (unlike java). Line 232: if ose.errno != errno.ENOENT: Line 233: self.log.error("%s file: %s can't be removed.", Line 234: fileType, filePath, exc_info=True) Line 235: Line 230: self.log.debug("%s file: %s was removed.", fileType, filePath) Line 231: except OSError as ose: Line 232: if ose.errno != errno.ENOENT: Line 233: self.log.error("%s file: %s can't be removed.", Line 234: fileType, filePath, exc_info=True) Do not use exc_info, it is non-standard and we want to kill it. If you want to log an exception (with traceback), use log.exception() In this context, missing file is very unlikely since we looked for all the files, so logging a warning about missing volume file is a good idea. Line 235: Line 236: def getAllVolumes(self): Line 237: """ Line 238: Return dict {volUUID: ((imgUUIDs,), parentUUID)} of the domain. -- To view, visit https://gerrit.ovirt.org/50804 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9eeb28a70f708a4f9a5effe4ff294da63b757369 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Idan Shaby <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
