Nir Soffer has posted comments on this change. Change subject: core: imageSharing - export logic to functions ......................................................................
Patch Set 7: Code-Review-1 (1 comment) http://gerrit.ovirt.org/#/c/26759/7/vdsm/storage/imageSharing.py File vdsm/storage/imageSharing.py: Line 105: toRead = min(BUFFER_SIZE, totalSize) Line 106: Line 107: try: Line 108: data = inFile.read(toRead) Line 109: except Exception as e: This is another change in the behavior that should not be here. Previously we handled here only socket.timeout (an expected error), or failed with a traceback. Now we handle all errors, and hide the traceback. Not specifying socket.timeout makes sense, making this code more generic, so it can be used to copy data from socket to file, or from file to socket, but there is no need to handle errors that read() cannot raise. Now since the error is less specific, it makes sense to log it in the error message. So this code should be: try: data = inFile.read(toRead) except IOError as e: log.error("error while attempting to read input data: %s", e) The motivation for making this function generic should be explained in the commit message. Line 110: log.error("error while attempting to read input data") Line 111: raise se.MiscFileReadException(str(e)) Line 112: Line 113: if not data: -- To view, visit http://gerrit.ovirt.org/26759 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I861b40cc62c3332b887b64c2525fc512cdc6a22a Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[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
