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

laravot at redhat.com laravot at redhat.com
Wed May 14 23:35:57 UTC 2014


Liron Ar has posted comments on this change.

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


Patch Set 1:

(6 comments)

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

Line 82:     p = utils.execCmd(cmd, sudo=False, sync=False,
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)
> even better, don't call the process variable "p", call it "process".
the method is supposed to be explanatory by it's name, not by the names of variables in the calls to it. therefore i'm leaving it as _waitProcessEnd
Line 87:     except Exception:
Line 88:         if p.returncode is None:
Line 89:             p.kill()
Line 90:         raise


Line 94:             p.kill()
Line 95:         raise
Line 96: 
Line 97: 
Line 98: def _ensureProcessSucceded(p, command, failureException):
> I agree with Nir, passing the exception as a parameter is ugly.
i left the raised exception as they were before, if you want to change it to general process exceptions we can look into that in a further patch, let's limit the scope here and not add "noise"
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()
> If you think that raising timeout will not be handled correctly on the engi
i left the raised exception as they were before, if you want to change it to general process exceptions we can look into that in a further patch, let's limit the scope here and not add "noise", this patch isn't about changing and testing changes to the exceptions here - the raised exceptions will be the same as they are before this change.
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()
> Same what?
i left the raised exception as they were before, if you want to change it to general process exceptions feel free to do so in a further patch, it's not related to this change.
Line 107: 
Line 108: 
Line 109: def _copyData(inBuffer, outBuffer, bytes_left):
Line 110:     total_size = bytes_left


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()
> I prefer is not a valid answer. You should explain why this is better.
this behavior change isn't related to this change - letting it blow could be changed in a different change with it being inspected and verified.
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 133:             raise se.MiscFileWriteException()
Line 134: 
Line 135:         bytes_left = bytes_left - len(data)
Line 136: 
Line 137:     outBuffer.close()
> No, this is not correct even in this context. This function did not open th
this function is private and used here only, i don't want to remember to close the buffer in each call. if it'll ever be public it can be removed, right now that's how i prefer to keep it.
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: 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