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