Change in vdsm[master]: core: imageSharing - export logic to functions
nsoffer at redhat.com
nsoffer at redhat.com
Sun May 25 22:35:17 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: core: imageSharing - export logic to functions
......................................................................
Patch Set 6:
(4 comments)
Mixing moving code *and* modifying the moved code in the same patch make this hard to review.
There are 3 logical changes here:
- Extracting the _copyData function, needed by the next patches - good
- More consistent naming style - good
- Pass the error string when raising exceptions - good
But why all these have to be in the same patch?
http://gerrit.ovirt.org/#/c/26759/6/vdsm/storage/imageSharing.py
File vdsm/storage/imageSharing.py:
Line 74: methodArgs.get("headers", {}))
Line 75:
Line 76:
Line 77: def streamDownloadImage(dstImgPath, methodArgs):
Line 78: totalSize = streamGetSize(methodArgs)
This is nice change, but not related to this patch.
Line 79: stream = methodArgs['fileObj']
Line 80:
Line 81: cmd = [constants.EXT_DD, "of=%s" % dstImgPath, "bs=%s" % constants.MEGAB]
Line 82: p = utils.execCmd(cmd, sudo=False, sync=False,
Line 99: raise
Line 100:
Line 101:
Line 102: def _copyData(inFile, outFile, totalSize):
Line 103: bytesToRead = totalSize
The name changes are not related to extracting this function.
Line 104: while totalSize > 0:
Line 105: toRead = min(BUFFER_SIZE, totalSize)
Line 106:
Line 107: try:
Line 107: try:
Line 108: data = inFile.read(toRead)
Line 109: except Exception as e:
Line 110: log.error("error while attempting to read input data")
Line 111: raise se.MiscFileReadException(str(e))
This change is good but not related to extracting this function.
Line 112:
Line 113: if not data:
Line 114: error = "partial data %s from %s" % \
Line 115: (bytesToRead - totalSize, bytesToRead)
Line 113: if not data:
Line 114: error = "partial data %s from %s" % \
Line 115: (bytesToRead - totalSize, bytesToRead)
Line 116: log.error(error)
Line 117: raise se.MiscFileReadException(error)
This change is good but not related to extracting this function.
Line 118:
Line 119: outFile.write(data)
Line 120: # outFile may not be a real file object but a wrapper.
Line 121: # To ensure that we don't use more memory as the input buffer size
--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list