Change in vdsm[master]: core: imageSharing - export logic to functions

nsoffer at redhat.com nsoffer at redhat.com
Mon Apr 28 21:06:57 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: core: imageSharing - export logic to functions
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.ovirt.org/#/c/26759/1/vdsm/storage/imageSharing.py
File vdsm/storage/imageSharing.py:

Line 78:     bytes_left = streamGetSize(methodArgs)
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,
sudo=False is the default, can remove this in a separate patch.
Line 83:                       deathSignal=signal.SIGKILL)
Line 84:     try:
Line 85:         _copyData(stream, p.stdin, bytes_left)
Line 86:         _ensureProcessSucceded(p, constants.EXT_DD, se.StorageException)


Line 94:             p.kill()
Line 95:         raise
Line 96: 
Line 97: 
Line 98: def _ensureProcessSucceded(p, command, failureException):
Passing the exception to raise is ugly. I don't see a need for this customization. 

The command is useful for nicer logs or exception text, but it is also ugly.

What we want there is an object encapsulating all the interesting info, and having a wait() method with a timeout and raising exceptions if the underlying process failed.

Having such object, we can get rid of the "stream" api (e.g. streamGetSize), and instead create an object in the upper layer and use it.
Line 99:     if not p.wait(WAIT_TIMEOUT):
Line 100:         log.error("timeout waiting for %s ", str(command))
Line 101:         raise failureException()
Line 102: 


Line 97: 
Line 98: def _ensureProcessSucceded(p, command, failureException):
Line 99:     if not p.wait(WAIT_TIMEOUT):
Line 100:         log.error("timeout waiting for %s ", str(command))
Line 101:         raise failureException()
Lets raise here ProcessTimeout() without the log.
Line 102: 
Line 103:     if p.returncode != 0:
Line 104:         log.error("%s error - code %s, stderr %s",
Line 105:                   str(command), p.returncode, p.stderr.read(1000))


Line 102: 
Line 103:     if p.returncode != 0:
Line 104:         log.error("%s error - code %s, stderr %s",
Line 105:                   str(command), p.returncode, p.stderr.read(1000))
Line 106:         raise failureException()
Lets raise here exception that works for every case:

    raise ProcessFailed("code %s, stderr %s",
			p.returncode, p.stderr.read(1000))
Line 107: 
Line 108: 
Line 109: def _copyData(inBuffer, outBuffer, bytes_left):
Line 110:     total_size = bytes_left


Line 105:                   str(command), p.returncode, p.stderr.read(1000))
Line 106:         raise failureException()
Line 107: 
Line 108: 
Line 109: def _copyData(inBuffer, outBuffer, bytes_left):
arguments names are not consistent. Please use the style in this file: bytesLeft.

Buffer is *not* a good name for the input and output file objects. The Python buffer interface is different from a file object and we don't want to confuse the readers. Better names are inFile and outFile or fromFile, toFile.
Line 110:     total_size = bytes_left
Line 111:     while bytes_left > 0:
Line 112:         to_read = min(BUFFER_SIZE, bytes_left)
Line 113: 


Line 114:         try:
Line 115:             data = inBuffer.read(to_read)
Line 116:         except Exception:
Line 117:             log.error("error while attempting to read input data")
Line 118:             raise se.MiscFileReadException()
The log adds nothing here, and the MiscFileReadException hide the original error, making it harder to debug later. Just don't handle this and let it blow.
Line 119: 
Line 120:         if not data:
Line 121:             log.error("partial data %s from %s",
Line 122:                       total_size - bytes_left, total_size)


Line 118:             raise se.MiscFileReadException()
Line 119: 
Line 120:         if not data:
Line 121:             log.error("partial data %s from %s",
Line 122:                       total_size - bytes_left, total_size)
Raise exception that explain the error and let the caller handle this:

    raise se.MiscFileReadException('partial data %s of %s bytes' % 
                                   (total_size - bytes_left, total_size))
Line 123:             raise se.MiscFileReadException()
Line 124: 
Line 125:         try:
Line 126:             outBuffer.write(data)


Line 126:             outBuffer.write(data)
Line 127:             # outBuffer may not be a real file object but a wrapper.
Line 128:             # To ensure that we don't use more memory as the input buffer size
Line 129:             # we flush on every write.
Line 130:             outBuffer.flush()
If we want to make it generic, lets remove this flush and let the caller decide on the size of file buffering policy.
Line 131:         except Exception:
Line 132:             log.exception("Error while writing data", exc_info=True)
Line 133:             raise se.MiscFileWriteException()
Line 134: 


Line 129:             # we flush on every write.
Line 130:             outBuffer.flush()
Line 131:         except Exception:
Line 132:             log.exception("Error while writing data", exc_info=True)
Line 133:             raise se.MiscFileWriteException()
Again, MiscFileWriteException hides the original exception, and since you log an exception here, we will have two exceptions in the log. The first will be useful, and the second totally useless.

To fix this, just let the original exception blow up, and let the caller log this.
Line 134: 
Line 135:         bytes_left = bytes_left - len(data)
Line 136: 
Line 137:     outBuffer.close()


Line 133:             raise se.MiscFileWriteException()
Line 134: 
Line 135:         bytes_left = bytes_left - len(data)
Line 136: 
Line 137:     outBuffer.close()
It is not correct to close files in this context. If this is a generic tool, the caller should have the freedom to close the files when she likes.
Line 138: 
Line 139: _METHOD_IMPLEMENTATIONS = {
Line 140:     'http': (httpGetSize, httpDownloadImage, httpUploadImage),
Line 141: }


-- 
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: 1
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan 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